Skip to content

testsuite: Add test for #9334 #9452

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

Closed
wants to merge 2 commits into from

Conversation

mpickering
Copy link
Collaborator

This adds a test which checks that v2-run works with profiling.

cabal v2-run --enable-profiling Main.hs

which is the same one as is run in ghcup-ci.

This test fails on recent GHC versions on GHCUp CI so hopefully also on cabal CI.

https://github.com/haskell/ghcup-metadata/actions/runs/6369779959/job/17290326630#step:3:15214

Copy link
Collaborator

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

👍

@mpickering mpickering force-pushed the wip/test-9334 branch 2 times, most recently from d166e76 to 0f655e2 Compare November 16, 2023 13:08
@ulysses4ever
Copy link
Collaborator

This test fails on recent GHC versions on GHCUp CI so hopefully also on cabal CI.

But it doesn't fail on our CI? I'm confused...

@mpickering
Copy link
Collaborator Author

This test fails on recent GHC versions on GHCUp CI so hopefully also on cabal CI.

But it doesn't fail on our CI? I'm confused...

I don't know.. it might only fail with 9.6.3 (which is not currently tested).

This adds a test which checks that v2-run works with profiling.

```
cabal v2-run --enable-profiling Main.hs
```

which is the same one as is run in ghcup-ci.

This test fails on recent GHC versions on GHCUp CI so hopefully also on
cabal CI.

https://github.com/haskell/ghcup-metadata/actions/runs/6369779959/job/17290326630#step:3:15214
@mpickering
Copy link
Collaborator Author

I rebased onto master, where 9.6.3 tests are run on windows, so we will perhaps see if there is a problem there!

@ulysses4ever
Copy link
Collaborator

No luck so far: windows-9.6.3 succeeded

@gbaz
Copy link
Collaborator

gbaz commented Nov 16, 2023

One wild guess.

The failing line is cabal --verbose=0 run --enable-profiling ./main.hs -- +RTS -s

I don't think the verbosity flag is the problem, and in sufficiently recent cabals run is the same as -run, but note that the faling line A) miscapitalizes main.hs, and B) calls it with a "./" prefix. I wonder if either of these is the cause of the failure?

@mpickering
Copy link
Collaborator Author

I tried to recreate that situation in the test. The file I added is main.hs (like the ghcup CI script) and also call it in the same way (with the ./ prefix).

I will further modify the test to look for Main.p_o .. because I imagine that actually main.p_o is getting created. It's also possible this is fixed on master, and broken on 3.10 branch.

@gbaz
Copy link
Collaborator

gbaz commented Nov 16, 2023

hrm, maybe we should try to diagnose also by prs against ghcup's ci that tweaks the calls a bit? its very hard to see what might be happening. are they using the same or different windows images?

Issue haskell#9334 identifies that `getScriptHash` can return invalid filepaths
on certain platforms. This randomised test attempts to check that the
filepaths it returns are valid.

I have verified that this test readily fails on linux (when the logic
about removing `/` is removed from ScriptUtils).
@mpickering
Copy link
Collaborator Author

I have added a second unit test which uses randomised testing to check the filepaths are valid.

@mpickering
Copy link
Collaborator Author

I deliberately rewrote my code away from the hlint suggestion so it was nicer to document the clauses. I don't intend to change it back.

@mpickering
Copy link
Collaborator Author

The test passes on windows, because % is in fact a valid character for a filepath. It just seems that some programs/contexts will interpret % as an environment variable marker. Do I augment my test with an ad-hoc check for % as well?

@ulysses4ever
Copy link
Collaborator

Re hlint: could you use {-# HLINT ignore "Use &&" #-} (or whatever the name of the suggestion, I haven't checked), so that the CI stays green, and you get your preferred style?

@ulysses4ever
Copy link
Collaborator

Do I augment my test with an ad-hoc check for % as well?

Yes, we live in a world where specs are not followed, and we have to acknowledge it, I think.

@jasagredo jasagredo mentioned this pull request Nov 18, 2023
5 tasks
@Mikolaj
Copy link
Member

Mikolaj commented Dec 21, 2023

@mpickering: would you like any help with that one?

@alt-romes
Copy link
Collaborator

Matthew thinks that this was ultimately not testing the right thing, so we are better off dropping the patch.

@alt-romes alt-romes closed this Mar 8, 2024
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.

7 participants