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

Test refactoring part 2 #463

Merged
merged 10 commits into from
Jan 25, 2024
Merged

Conversation

mjcarroll
Copy link
Contributor

Further cleanups to testing after the subprocess changes.

This does:

  • Pushes auxillary test executables down one directory, just to tidy up the integration folder and makes it clear which things are tests vs support executables
  • Splits "test utils" (utility code) from "test config" (generated configuration)
  • Puts auxillary executable definitions in one location

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Dec 1, 2023
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ca954cb) 87.23% compared to head (b9fb754) 87.27%.
Report is 1 commits behind head on gz-transport12.

Additional details and impacted files
@@                Coverage Diff                 @@
##           gz-transport12     #463      +/-   ##
==================================================
+ Coverage           87.23%   87.27%   +0.03%     
==================================================
  Files                  60       60              
  Lines                5211     5211              
==================================================
+ Hits                 4546     4548       +2     
+ Misses                665      663       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM, just a small comment about two std includes

test/test_utils.hh Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor Author

There is still a failing homebrew test (that I think is new) and also Windows is failing to clone the repo (@j-rivero?)

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll mjcarroll self-assigned this Dec 15, 2023
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll
Copy link
Contributor Author

I found the culprit here, I accidentally switched out the test executables from one that runs indefinitely to one that exits after a few seconds. It seems that on Linux runners, we are fast enough to complete the stress test in the window, where on other platforms we are too slow.

I have switched the test back accordingly.

@mjcarroll
Copy link
Contributor Author

@osrf-jenkins retest this please

@mjcarroll mjcarroll closed this Jan 7, 2024
@mjcarroll mjcarroll reopened this Jan 7, 2024
@caguero
Copy link
Collaborator

caguero commented Jan 8, 2024

Do you know if the Windows failure is related to this PR?

@mjcarroll
Copy link
Contributor Author

Do you know if the Windows failure is related to this PR?

I'm not sure. On one hand I haven't seen it before, so maybe my fault, but also seems unrelated to the changes in the PR?

@mjcarroll
Copy link
Contributor Author

Looks like there are actual multiple failing tests, but they aren't being reported correctly:

The following tests FAILED:
	 47 - INTEGRATION_twoProcsSrvCallStress (Timeout)
	 53 - INTEGRATION_twoProcsSrvCallWithoutInputStress (Timeout)
	 59 - INTEGRATION_twoProcsSrvCallWithoutOutputStress (Timeout)
	 83 - INTEGRATION_playback (Failed)
Errors while running CTest

@mjcarroll
Copy link
Contributor Author

Looks like Windows failure may have been a flake. I will merge this and continue to monitor.

@mjcarroll mjcarroll merged commit 92d02dc into gz-transport12 Jan 25, 2024
11 checks passed
@mjcarroll mjcarroll deleted the mjcarroll/test_cleanup_part2 branch January 25, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants