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

test-codegen: replace tempname/open/close/stat by mktemp #23365

Closed
wants to merge 2 commits into from
Closed

test-codegen: replace tempname/open/close/stat by mktemp #23365

wants to merge 2 commits into from

Conversation

tkluck
Copy link
Contributor

@tkluck tkluck commented Aug 20, 2017

This seems like a flaky test on Windows,

    FAILURE
Error in testset codegen:
Test Failed
  Expression: tempty == true
   Evaluated: false == true
ERROR: LoadError: Test run finished with errors
while loading C:\projects\julia\julia-a661736be5\share\julia\test\runtests.jl, in expression starting on line 29
Command exited with code 1

and without being able to reproduce, I'm going to guess it's a race condition betweeen close and stat. If so, replacing the stat (2) syscall by fstat (2) may fix it. At the Julia level, that means calling stat on the iostream instead.

@tkluck
test-codegen: replace tempname/open/close/stat by mktemp
d7e52d8 
This seems like a flaky test on Windows [1],

        FAILURE
    Error in testset codegen:
    Test Failed
      Expression: tempty == true
       Evaluated: false == true
    ERROR: LoadError: Test run finished with errors
    while loading C:\projects\julia\julia-a661736be5\share\julia\test\runtests.jl, in expression starting on line 29
    Command exited with code 1

and without being able to reproduce, I'm going to guess it's a race
condition betweeen `close` and `stat`. If so, replacing the `stat (2)`
syscall by `fstat (2)` may fix it.  At the Julia level, that means
calling `stat` on the iostream instead.

[1]: https://ci.appveyor.com/project/JuliaLang/julia/build/1.0.17932/job/kn10okpwqrbs0126
@tkluck tkluck mentioned this pull request Aug 20, 2017
Merged
@kshyatt kshyatt added compiler:codegen Generation of LLVM IR and native code test This change adds or pertains to unit tests system:windows Affects only Windows labels Aug 20, 2017
tkelman
tkelman reviewed Aug 21, 2017
View reviewed changes
test/codegen.jl Outdated
test_jl_dump_compiles_internal(1)
ccall(:jl_dump_compiles, Void, (Ptr{Void},), C_NULL)

@test stat(io).size == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this changing the sense of the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm you think? Maybe I'm missing something, but it seemed to me the intention is to test what gets written to the io handle that we pass to :jl_dump_compiles -- namely, test that nothing gets written (and/or (unlikely but possible) that everything gets truncated after writing). In any case, shouldn't that be the same between calling fstat on the file handle and calling stat on the file name?

Copy link
Member

Choose a reason for hiding this comment

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

I think what @tkelman meant is that tempty == false was equivalent to tstats.size != 0 in the existing code. Now you made it test that this is == 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! I missed that the two test cases were different. Thanks for pointing that out, @nalimilan and @tkelman . In addition, the reason why the test case still passed is that fstat only gives the correct size after flush.

Let's see what AppVeyor says.

@tkluck
test-codegen: flush() before fstat()
2db4b47 
This fixes two issues:

1. I replaced an `tempty == false` by `size == 0` (should have been `size != 0`)
2. I didn't notice that because I didn't `flush()` before `stat`ing the handle.

The second point is ironic, since that's exactly the kind of issue this
branch is trying to avoid! See commit message for d7e52d8 for
details.
@fredrikekre
Copy link
Member

Think this happened on Travis too https://gist.github.com/fredrikekre/ee13a0a48230346343da48e721c0b5c4

	From worker 1:		From worker 1:	Worker 10 failed running test codegen:
Some tests did not pass: 86 passed, 1 failed, 0 errored, 0 broken.codegen: Test Failed
  Expression: tempty == true
   Evaluated: false == true
Stacktrace:
 [1] record(::Base.Test.DefaultTestSet, ::Base.Test.Fail) at ./test.jl:651
 [2] (::##40#44)() at /private/tmp/julia/share/julia/test/runtests.jl:160
 [3] cd(::##40#44, ::String) at ./file.jl:70
 [4] include_relative(::Module, ::String) at ./loading.jl:464
 [5] include(::Module, ::String) at ./sysimg.jl:14
 [6] process_options(::Base.JLOptions) at ./client.jl:315
 [7] _start() at ./client.jl:383

@fredrikekre fredrikekre mentioned this pull request Aug 25, 2017
Merged
@vtjnash vtjnash mentioned this pull request Aug 28, 2017
Merged
@fredrikekre
Copy link
Member

Subsumed by #23486 ?

@tkluck
Copy link
Contributor Author

tkluck commented Aug 29, 2017

@fredrikekre looks like it! Closing.

@tkluck tkluck closed this Aug 29, 2017
@tkluck tkluck deleted the mktemp-in-test-codegen branch January 6, 2018 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nalimilan nalimilan nalimilan left review comments

@tkelman tkelman tkelman left review comments

Assignees
No one assigned
Labels
compiler:codegen Generation of LLVM IR and native code system:windows Affects only Windows test This change adds or pertains to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tkluck @fredrikekre @nalimilan @tkelman @kshyatt