Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests that simply run all (SIL) configs in everest-core/config #437

Merged
merged 16 commits into from
Feb 9, 2024

Conversation

hikinggrass
Copy link
Contributor

@hikinggrass hikinggrass commented Nov 17, 2023

This should help in finding regressions that need configuration changes or detect broken configs

@hikinggrass hikinggrass force-pushed the kh-add-simple-config-tests branch from 023fe68 to cf55dcd Compare November 20, 2023 09:50
Copy link
Contributor

@Dominik-K Dominik-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multiplied, same-functionality code should be generalized (see comment).

tests/core_tests/config_tests.py Outdated Show resolved Hide resolved
tests/core_tests/config_tests.py Outdated Show resolved Hide resolved
.ci/e2e/scripts/tests.sh Outdated Show resolved Hide resolved
@Dominik-K
Copy link
Contributor

How long will these tests take? Tried to answer myself but the tests are currently failing on initialisation.

If it takes too long, one option would be to run them in parallel. But unfortunately, GitHub Actions doesn't support this out of the box, AFAIK. CircleCI has a neat helper for it. But perhaps there's a good tool for GitHub Actions, too.

@Dominik-K Dominik-K closed this Nov 20, 2023
@Dominik-K
Copy link
Contributor

Sorry, clicked wrong 🙈

@Dominik-K Dominik-K reopened this Nov 20, 2023
@hikinggrass
Copy link
Contributor Author

How long will these tests take? Tried to answer myself but the tests are currently failing on initialisation.

If it takes too long, one option would be to run them in parallel. But unfortunately, GitHub Actions doesn't support this out of the box, AFAIK. CircleCI has a neat helper for it. But perhaps there's a good tool for GitHub Actions, too.

Let's see how long they take, them failing right now is already being worked on (basically we use a version of everest-testing that's too old in the ci tests)

@Dominik-K
Copy link
Contributor

If it takes too long, one option would be to run them in parallel. But unfortunately, GitHub Actions doesn't support this out of the box, AFAIK. CircleCI has a neat helper for it. But perhaps there's a good tool for GitHub Actions, too.

Yeah, there are tools for GitHub Actions. Was quite simple to find searching for "split tests":

@hikinggrass
Copy link
Contributor Author

All 15 tests that are implemented right now run sequentially and take ~67 seconds, so <5s/test. Codacy for example usually takes much longer than that, so I think this is negligible for now.

@hikinggrass hikinggrass force-pushed the kh-add-simple-config-tests branch from 43b21fa to 9c54906 Compare November 21, 2023 11:06
Copy link
Contributor

@Dominik-K Dominik-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with comments (in the meaning: I'm sure you'll address my comments.)

tests/conftest.py Outdated Show resolved Hide resolved
tests/core_tests/config_tests.py Show resolved Hide resolved
tests/core_tests/config_tests.py Outdated Show resolved Hide resolved
tests/core_tests/config_tests.py Show resolved Hide resolved
@hikinggrass hikinggrass force-pushed the kh-add-simple-config-tests branch 2 times, most recently from 9b2b170 to d46a0aa Compare December 8, 2023 08:04
@corneliusclaussen corneliusclaussen force-pushed the kh-add-simple-config-tests branch from d46a0aa to 189b7c0 Compare December 12, 2023 08:38
@corneliusclaussen corneliusclaussen force-pushed the kh-add-simple-config-tests branch from 189b7c0 to fb36b0e Compare December 22, 2023 11:11
@hikinggrass
Copy link
Contributor Author

For this PR to work we need to have these merged:
#475
EVerest/everest-ci#12

@hikinggrass hikinggrass force-pushed the kh-add-simple-config-tests branch 2 times, most recently from ceceaba to 5f04f34 Compare December 22, 2023 15:28
@corneliusclaussen corneliusclaussen force-pushed the kh-add-simple-config-tests branch from fdd926f to 29e80e3 Compare January 18, 2024 15:37
@hikinggrass hikinggrass force-pushed the kh-add-simple-config-tests branch from 29e80e3 to 2e538ca Compare February 7, 2024 15:08
@hikinggrass hikinggrass force-pushed the kh-add-simple-config-tests branch 2 times, most recently from d406dbb to fcd141d Compare February 8, 2024 18:54
@hikinggrass hikinggrass requested a review from andistorm February 8, 2024 19:26
tests/README.md Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@hikinggrass hikinggrass force-pushed the kh-add-simple-config-tests branch from 90f99e3 to cdedd6c Compare February 9, 2024 09:18
Copy link
Contributor

@andistorm andistorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

Co-authored-by: Andreas Heinrich <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
@hikinggrass hikinggrass force-pushed the kh-add-simple-config-tests branch from cdedd6c to aefeb59 Compare February 9, 2024 09:28
@hikinggrass hikinggrass merged commit 3d4792c into main Feb 9, 2024
5 checks passed
@hikinggrass hikinggrass deleted the kh-add-simple-config-tests branch February 9, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants