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 functional tests, fix unset exit codes on panics #42

Merged
merged 16 commits into from
Feb 14, 2023

Conversation

JohnStarich
Copy link
Contributor

I ran into a few panics in my projects and realized those triggered package failures but were "passing" the tests because they exited with 0.

This PR fixes those exit codes so it doesn't miss anything.

I had difficulty reproducing the failures for a while, which necessitated this new functional test style. I hope it is to your liking! 😄

  • Refactor to support functional tests
  • Fix unset exit code from uncaught exception
  • Fix unset exit code for panic in next tick of event loop

main.go Outdated Show resolved Hide resolved
@agnivade agnivade self-requested a review January 25, 2023 04:58
@JohnStarich
Copy link
Contributor Author

Bummer. Windows tests are failing with the same issue as #40

@JohnStarich
Copy link
Contributor Author

I believe we can fix the Windows issue with this: #40 (comment)
It's not great, but it does work. Hopefully we can resolve that issue.

@agnivade
Copy link
Owner

I haven't been able to find time to get to review this. It's on my list, thank you for your patience!

@JohnStarich
Copy link
Contributor Author

@agnivade Looks like cleanenv worked! I also had to bump the test timeout because Windows is slower.

We're all 🟢 now!

Copy link
Owner

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!

parse_test.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Show resolved Hide resolved
Copy link
Owner

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Minor cleanup comments. Looking great overall :)

parse_test.go Outdated Show resolved Hide resolved
http_server.go Outdated Show resolved Hide resolved
@JohnStarich
Copy link
Contributor Author

Thanks for the reviews!
That Windows 1.19 test run is constantly flaking. I wonder if it’s getting overwhelmed somehow. Would you mind giving it a restart, as I don’t have access?

@agnivade
Copy link
Owner

I have a feeling this is going to remain flaky. I have restarted for now, but if this continues, we might want to skip the test in Windows.

@agnivade agnivade merged commit 2d1bbcf into agnivade:master Feb 14, 2023
@JohnStarich JohnStarich deleted the feature/fix-unhandled-test-panic branch February 15, 2023 04:38
@JohnStarich
Copy link
Contributor Author

Thank you for the merge! Would you mind cutting a new release when you have a chance?

That's fair. It looked to me like it wasn't always the same test that failed on Windows – I'm not certain though. It's possible too many tests were running in parallel, so we might consider go test -parallel N.

@agnivade
Copy link
Owner

Yeah, good point on the parallelism. I think we might be going a bit overboard on that. I am leaning towards simply removing t.Parallel to keep things simple.

@agnivade
Copy link
Owner

Btw, I've cut a new release.

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.

2 participants