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

Fix flaky test by a test env setup refactor #4273

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

komikat
Copy link
Contributor

@komikat komikat commented Jun 2, 2024

Fixes #4200 by creating a separate temporary .cache directory for every test instead of having all tests share the same .cache directory. This makes sure the test iface-error-test-1 is run with a clean directory — causing the diagnostic to be generated every time.

@komikat komikat requested a review from fendor as a code owner June 2, 2024 20:33
@soulomoon
Copy link
Collaborator

soulomoon commented Jun 3, 2024

It seems hls-refactor-plugin-tests depend on the cache directory.🤔

/tmp/hls-test-root/.cache-b4d2c0447616dd51: createDirectory: does not exist (No such file or directory)

@fendor
Copy link
Collaborator

fendor commented Jun 3, 2024

That seems impossible to me, there must be another race condition :D

@komikat
Copy link
Contributor Author

komikat commented Jun 3, 2024

Weird because hls-refactor-plugin-tests seems to pass on my system (MacOS, ghc9.8)

@komikat komikat marked this pull request as draft June 3, 2024 20:29
@komikat
Copy link
Contributor Author

komikat commented Jun 3, 2024

The setup wasn't creating the hls-test-root directory.
ghcide-tests is still failing, will look into this soon.

@komikat
Copy link
Contributor Author

komikat commented Jun 18, 2024

definition.Imported symbol (reexported) fails without $TEMPDIR/hls-test-root

running HLS_TEST_HARNESS_NO_TESTDIR_CLEANUP=1 HLS_TEST_LOG_STDERR=1 TASTY_PATTERN="/definition.Imported symbol (reexported)/" cabal test ghcide-tests
FAILS (>=GHC94)

expected: Location {_uri = Uri {getUri = "$TMPDIR/hls-test-root/extra-dir-85406152527/Bar.hs"}, _range = Range {_start = Position {_line = 3, _character = 5}, _end = Position {_line = 3, _character = 8}}}
 but got: Location {_uri = Uri {getUri = "$TMPDIR/hls-test-root/extra-dir-85406152527/Bar.hs"}, _range = Range {_start = Position {_line = 3, _character = 0}, _end = Position {_line = 3, _character = 14}}}

Notes

  • AtPoint.hs.gotoDefinition calls locationsAtPoint
  • it is using nameToLocation when failing
  • itExists <- liftIO $ doesFileExist fs returns true
  • doesn't seem to be falling back to db
  • in srcSpanToLocation sp
    rng <- srcSpanToRange src
    reports a different location from what's expected in the test
  • the sp comes from
    nameSrcSpan name
    in that branch.

@fendor
Copy link
Collaborator

fendor commented Jun 26, 2024

This is good!

Let's merge this as is, add the comment to the test case, create an issue containing the knowledge you have gained so far, and then accept the test case changes. E.g., update the assertions in the test. Even though we have discovered an interesting race (?) condition, the test case itself will be stable.

@komikat Could you do that, please?

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Please add a comment to the test case that fails now and create a new tracking issue! LGTM otherwise

@soulomoon
Copy link
Collaborator

definition.Imported symbol (reexported) fails without $TEMPDIR/hls-test-root

running HLS_TEST_HARNESS_NO_TESTDIR_CLEANUP=1 HLS_TEST_LOG_STDERR=1 TASTY_PATTERN="/definition.Imported symbol (reexported)/" cabal test ghcide-tests FAILS (>=GHC94)

expected: Location {_uri = Uri {getUri = "$TMPDIR/hls-test-root/extra-dir-85406152527/Bar.hs"}, _range = Range {_start = Position {_line = 3, _character = 5}, _end = Position {_line = 3, _character = 8}}}
 but got: Location {_uri = Uri {getUri = "$TMPDIR/hls-test-root/extra-dir-85406152527/Bar.hs"}, _range = Range {_start = Position {_line = 3, _character = 0}, _end = Position {_line = 3, _character = 14}}}

Notes

  • AtPoint.hs.gotoDefinition calls locationsAtPoint
  • it is using nameToLocation when failing
  • itExists <- liftIO $ doesFileExist fs returns true
  • doesn't seem to be falling back to db
  • in srcSpanToLocation sp
    rng <- srcSpanToRange src
    reports a different location from what's expected in the test
  • the sp comes from
    nameSrcSpan name
    in that branch.

This is a bug in our code, It worth a seperate issue to fix this.

@komikat
Copy link
Contributor Author

komikat commented Jul 5, 2024

The test has this condition which seems to be wrong

if ghcVersion >= GHC94 && ghcVersion < GHC910 then mkL bar 3 5 3 8 else mkL bar 3 0 3 14

I've set it to just be 3 0 3 14 for all tests for now, since that's what the new test suite seems to accept.

@komikat komikat marked this pull request as ready for review July 5, 2024 13:09
@komikat komikat requested a review from wz1000 as a code owner July 5, 2024 13:09
@komikat
Copy link
Contributor Author

komikat commented Jul 5, 2024

Hm, still failing a few windows tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants