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.

Sign up for GitHub

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

Jump to bottom

Fix problem where everything would be suppressed forever #18

Merged
merged 1 commit into from
Feb 23, 2018
Merged

Fix problem where everything would be suppressed forever #18

merged 1 commit into from
Feb 23, 2018

Conversation

iamed2
Copy link
Contributor

@iamed2 iamed2 commented Feb 22, 2018

This PR fixes a problem where everything would be suppressed forever if an error occurred in the suppressed block. The tests were actually failing (the tests themselves were incorrect) but no one noticed because the failures were suppressed 🙃

Also changed @async to @schedule as there is no enclosing @sync.

@iamed2 iamed2 mentioned this pull request Feb 22, 2018
Merged
ssfrr
ssfrr approved these changes Feb 23, 2018
View reviewed changes
Copy link
Collaborator

@ssfrr ssfrr left a comment

Choose a reason for hiding this comment

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

LGTM

test/runtests.jl Outdated
@@ -50,19 +52,8 @@ end == 42
@test_throws ErrorException @suppress begin
println("This string doesn't get printed!")
warn("This warning is ignored.")
error("Remember that errors are still printed!")
error("Remember that errors are still printed!") # they aren't; @test_throws suppresses them
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this string be changed then to something like "errors would normally get printed but are caught here by @test_throws" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@ssfrr
Copy link
Collaborator

ssfrr commented Feb 23, 2018

failure on 0.5 looks unrelated, and maybe it's OK to drop 0.5 support...

Not sure what's up with the 0.7 failure - can anyone repro it locally?

@iamed2
Copy link
Contributor Author

iamed2 commented Feb 23, 2018

Yup, happens to me locally too. Here it is parsed out of the messy log:

Suppressor: Test Failed at /Users/ericdavies/.julia/v0.7/Suppressor/test/runtests.jl:26
  Expression: output == "\e[1m\e[33mWARNING: \e[39m\e[22m\e[33mshould get captured, not printed\e[39m\n"
   Evaluated: "WARNING: should get captured, not printed\n" == "\e[1m\e[33mWARNING: \e[39m\e[22m\e[33mshould get captured, not printed\e[39m\n"
Stacktrace:
 [1] macro expansion at /Users/ericdavies/.julia/v0.7/Suppressor/test/runtests.jl:26 [inlined]
 [2] macro expansion at /Users/ericdavies/repos/juliamaster/usr/share/julia/site/v0.7/Test/src/Test.jl:1012 [inlined]
 [3] top-level scope at /Users/ericdavies/.julia/v0.7/Suppressor/test/runtests.jl:6

All the other tests pass and the bug is unrelated, so I think this PR is good.

@Ismael-VC
Copy link
Contributor

Ismael-VC commented Feb 23, 2018 •
edited
Loading

We should also test on Windows, see: #19

I don't know if someone setup Travis or if it just worked after the ownership transfer, in case this was automatic, we'd need to setup a JuliaIO account for AppVeyor.

@Ismael-VC
Copy link
Contributor

@iamed2 I have enabled AppVeyor, could you please submit a commit with any minor change, to a comment perhaps, just to trigger the Windows CI builds?

@Ismael-VC Ismael-VC added the bug label Feb 23, 2018
@iamed2 iamed2 closed this Feb 23, 2018
@iamed2 iamed2 reopened this Feb 23, 2018
@iamed2
Copy link
Contributor Author

iamed2 commented Feb 23, 2018

AppVeyor nightly tests fail because the Windows binaries don't exist (known issue)

@Ismael-VC Ismael-VC merged commit 3e1680d into JuliaIO:master Feb 23, 2018
@iamed2 iamed2 deleted the fix-errors branch February 24, 2018 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ismael-VC Ismael-VC Ismael-VC approved these changes

@ssfrr ssfrr ssfrr approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iamed2 @ssfrr @Ismael-VC