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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable integration test workflow to communicate failure #738

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mfisher87
Copy link
Member

@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
Member Author

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

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
Member 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration tests failing silently
2 participants