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

Enable integration test workflow to more clearly communicate failure #738

Merged
merged 5 commits into from
Jul 9, 2024

Conversation

mfisher87
Copy link
Collaborator

@mfisher87 mfisher87 commented Jun 30, 2024

Resolves #737

I don't have free time to debug the actual failures. Just getting the tests to fail loudly was a headache 😆 At a glance, I see a couple problems:

  • We have some assertions at the module level that look like they should be set up in fixtures
  • We're using unittest assert functions, so pytest is providing very poor error messages

@mfisher87
Copy link
Collaborator Author

Returned to draft; it was failing loudly in Act, but looks like still failing silently in GHA.

betolink
betolink previously approved these changes Jul 2, 2024
Copy link
Member

@betolink betolink left a comment

Choose a reason for hiding this comment

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

Thanks for noticing, I don't know when we started to exclude the integration tests!

@betolink betolink marked this pull request as ready for review July 2, 2024 23:59
@betolink betolink self-requested a review July 2, 2024 23:59
@betolink betolink marked this pull request as draft July 3, 2024 00:00
@betolink
Copy link
Member

betolink commented Jul 3, 2024

Is there any reason not to merge? if the tests fail we should fix them. Maybe we need to visit the option to define a few popular datasets per DAAC instead of using random samples because some of them fail.

@mfisher87
Copy link
Collaborator Author

I think at minimum we need to update the fixture that allows 10% failure to communicate clearly that it was invoked before merging this one. Since it's a lot of work to rework the tests to remove that rule, this seems like a reasonable step. I'll try and take a look at that soon.

@mfisher87 mfisher87 changed the title Enable integration test workflow to communicate failure Enable integration test workflow to better communicate failure Jul 6, 2024
@mfisher87
Copy link
Collaborator Author

mfisher87 commented Jul 6, 2024

@betolink please take a look!

  • I changed the hook so that instead of swallowing the failure, it uses a special error code 99. This enables us to tell the difference between a success, an acceptable failure, and an unacceptable failure.
  • I changed the hook so that it only acts on error code 1; other error codes should not be modified because they indicate something other than "some tests failed". See comments for more!
  • I changed the integration test script to do the "swallowing" and clearly communicate why. We can't use the pytest hook for this because then the print happens above the error summary and the user won't be able to find it among the logspew.

@mfisher87 mfisher87 marked this pull request as ready for review July 6, 2024 16:46
@mfisher87 mfisher87 added the hackathon An issue we'd like to visit during a hackathon event label Jul 8, 2024
@mfisher87
Copy link
Collaborator Author

pre-commit.ci autofix

@itcarroll itcarroll self-requested a review July 9, 2024 17:46
@mfisher87 mfisher87 merged commit d4fd6a6 into main Jul 9, 2024
17 checks passed
@mfisher87 mfisher87 deleted the fix-integration-test-silent-failure branch July 9, 2024 17:48
@mfisher87 mfisher87 changed the title Enable integration test workflow to better communicate failure Enable integration test workflow to more clearly communicate failure Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hackathon An issue we'd like to visit during a hackathon event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration tests failing silently
3 participants