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

Make CI great again #308

Merged
merged 8 commits into from
Jul 9, 2024

Conversation

geekingfrog
Copy link
Contributor

Alternative title: declare test bankruptcy.

This PR adds a tag to ignore all currently failing tests. The output of mix test --exclude needs_attention is still drowned in error logs, but at least all tests are passing. This is making the CI step useful, if a known good test starts to fail, people will notice.

And then we can start having a look at slowly removing these tags and fixing the tests. There is really a mix bags:

  • Some very simple tests were generated once and obviously never run ever again, can probably delete them
  • Some tests are about the old tachyon protocol based on plain tls connnection. These can be removed alongside the old tachyon code
  • Some tests haven't been updated when the code changed, typically, spring protocol change/extension, or html output
  • Some tests are exercising code looking at dirty cache and failing because of that

@geekingfrog
Copy link
Contributor Author

Should probably wait for #304 to be merged since the different otp/elixir version seems to be causing some issues.

@StanczakDominik StanczakDominik marked this pull request as ready for review June 9, 2024 09:02
@StanczakDominik StanczakDominik marked this pull request as draft June 9, 2024 09:11
@StanczakDominik
Copy link
Collaborator

StanczakDominik commented Jun 9, 2024

I'm still getting 29 failures locally, 3 more than here in CI, when calling MIX_ENV=test mix do ecto.drop + test --exclude needs_attention :|

@geekingfrog
Copy link
Contributor Author

I'm still getting 29 failures locally, 3 more than here in CI, when calling MIX_ENV=test mix do ecto.drop + test --exclude needs_attention :|

Yeah, this needs more work, I'll get to it at some point this week

@geekingfrog
Copy link
Contributor Author

riiiight, so this passes locally, but not in CI, that one isn't going to be fun

@StanczakDominik
Copy link
Collaborator

Maybe even have multiple separate tags for those? Probably overkill and silly, but something to consider.

@geekingfrog geekingfrog marked this pull request as ready for review June 27, 2024 20:59
@geekingfrog
Copy link
Contributor Author

Alright, I managed to reproduce the errors.
I "fixed" them by disabling even more tests. They were all related to (old) tachyon stuff so I don't feel too bad. These tests are likely going to get deleted soon, once we get something working with the ws based tachyon.

Because that's where they should live.
And also, because there are some reference to ExUnit in this file now,
that is only available during tests. Putting the test_lib file under
test makes dialyzer happier since it's on the correct compiler path.
@geekingfrog geekingfrog force-pushed the ignore-many-tests branch 2 times, most recently from c828de5 to f0c3e08 Compare July 4, 2024 08:02
Copy link
Member

@jauggy jauggy left a comment

Choose a reason for hiding this comment

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

Nice looks like CI Tests all pass. Even though some tests have been disabled it at least makes CI useful again.

@geekingfrog geekingfrog mentioned this pull request Jul 5, 2024
6 tasks
@L-e-x-o-n L-e-x-o-n merged commit 72b81e6 into beyond-all-reason:master Jul 9, 2024
3 checks passed
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.

4 participants