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

Update TestRestart - CrossOS Passing & More Consistent #125

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

mxygem
Copy link
Contributor

@mxygem mxygem commented Dec 5, 2021

Noticed that this test wasn't passing for me when running things earlier. If there's too much timing variance in execution it can throw false-positives at least in part due to the way the test is asserting correctness.

The current flow of the test is:

  1. Create a spinner
  2. Start spinner & sleep for a period
  3. Restart spinner & sleep for another period
  4. Stop spinner
  5. Split the output bytes in half and compare them to one another

Even though the spinner successfully restarted the test can fail if the count is off and (I think) can also be exacerbated by ANSI codes.

I had done some larger WIP testing where I would strip the output of control characters with the idea being to compare the output spinner characters directly. Doing something like that wouldn't totally address for variance still. Changing to check that the subset of symbols written after the restart also exists in the original run could help. Or perhaps simply that there were even any characters written after restart?

@briandowns
Copy link
Owner

Is this test now running more consistently?

@mxygem
Copy link
Contributor Author

mxygem commented Dec 5, 2021

Yep! I've tested it on my windows machine with -race and -count=30 across cmd, powershell, and wsl. My machine's hardware is pretty solid though so that may impact things. I haven't yet tried it on mac yet.

@briandowns
Copy link
Owner

That works for me. Thank you! If you want to test it on Mac, ok, otherwise I'm fine if you want this merged.

@mxygem
Copy link
Contributor Author

mxygem commented Dec 6, 2021

I've been able to run this on an M1 MBP consistently without any issues. Ran this a few times:

go test -timeout 30s -race -count=30 -run ^TestRestart$ github.com/briandowns/spinner

Good to merge. :)

@briandowns briandowns merged commit 8cd3c74 into briandowns:master Dec 6, 2021
@briandowns
Copy link
Owner

Thanks for the contribution!

@mxygem mxygem deleted the update-test-restart branch December 6, 2021 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants