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

Add test-wrapper option to wrap test execution #5995

Merged
merged 12 commits into from
Apr 30, 2019

Conversation

angerman
Copy link
Collaborator

Sometimes we need to run the test ins some prebuild context, for
these cases we now allow a script/application to be passed as
--test-wrapper, which will obtain the execution arguments and
be responsible for running the test in the environment.

A test-wrapper could for example do the following to execute a
test through wine:

set -euo pipefail
WINEDLLOVERRIDES="winemac.drv=d" \
  WINEDEBUG=-all+error \
  LC_ALL=en_US.UTF-8 \
  WINEPREFIX=$(mktemp -d) \
  /path/to/bin/wine64 $@*

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Sometimes we need to run the test ins some prebuild context, for
these cases we now allow a script/application to be passed as
`--test-wrapper`, which will obtain the execution arguments and
be responsible for running the test in the environment.

A test-wrapper could for example do the following to execute a
test through wine:

```
set -euo pipefail
WINEDLLOVERRIDES="winemac.drv=d" \
  WINEDEBUG=-all+error \
  LC_ALL=en_US.UTF-8 \
  WINEPREFIX=$(mktemp -d) \
  /path/to/bin/wine64 $@*
```
@angerman angerman requested review from hvr, harpocrates and phadej April 10, 2019 03:02
@angerman angerman self-assigned this Apr 10, 2019
@23Skidoo
Copy link
Member

Looks fine in principle, but we should think about what happens when cabal test passes --test-wrapper to a custom Setup script compiled againt Cabal < 3.0. We may need add a variant of filterConfigureFlags for the test command.

Also needs a changelog note and documentation, you can add the latter here:

Copy link
Collaborator

@harpocrates harpocrates left a comment

Choose a reason for hiding this comment

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

I think this is still WIP, right? Even after fixing some more obvious issues, I wasn't able to reproduce the expected behaviour (the wrapper script I passed in was being ignored).

Might be helpful to mark as WIP until ready for actual review. 🙂

@@ -294,6 +294,7 @@ data ElaboratedConfiguredPackage
elabTestHumanLog :: Maybe PathTemplate,
elabTestShowDetails :: Maybe TestShowDetails,
elabTestKeepTix :: Bool,
elabTestWrapper :: Maybe FilePath
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing trailing comma means this doesn't compile.

@@ -1884,6 +1884,7 @@ elaborateInstallPlan verbosity platform compiler compilerprogdb pkgConfigDB
elabTestHumanLog = perPkgOptionMaybe pkgid packageConfigTestHumanLog
elabTestShowDetails = perPkgOptionMaybe pkgid packageConfigTestShowDetails
elabTestKeepTix = perPkgOptionFlag pkgid False packageConfigTestKeepTix
elabTestWrapper = perPkgOptionFlag pkgid False packageConfigTestWrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean perPkgOptionMaybe pkgid packageConfigTestWrapper?

"Run test through a wrapper."
testWrapper (\v flags -> flags { testWrapper = v })
(reqArg' "FILE" (toFlag :: FilePath -> Flag FilePath)
(flagToList :: Flag FilePath -> [FilePath]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to add this to at least Distribution.Cabal.Setup.testOptions if you want it to actually make it through to a user-facing option.

@angerman
Copy link
Collaborator Author

I think this is still WIP, right? Even after fixing some more obvious issues, I wasn't able to reproduce the expected behaviour (the wrapper script I passed in was being ignored).

Might be helpful to mark as WIP until ready for actual review. 🙂

This is actually being used with the low level plumbing of the Setup.hs, so yes there might be something missing for the cabal-install frontend :-)

@JoshMeredith
Copy link
Collaborator

JoshMeredith commented Apr 24, 2019

We may need add a variant of filterConfigureFlags for the test command.

What would be the version number used in the code for filterTestFlags, given that the project files use a placeholder 3.0.0 version number?

E.g. https://github.com/zw3rk/cabal/blob/db8745cc63f2aff73afd5ed208cb839be552c8e2/cabal-install/Distribution/Client/Setup.hs#L512

@23Skidoo
Copy link
Member

Using 3.0.0.0 is fine.

@23Skidoo
Copy link
Member

@JoshMeredith There are some CI failures still unfortunately.

@23Skidoo
Copy link
Member

Looks like some QuickCheck tests are now failing.

@JoshMeredith
Copy link
Collaborator

Are the current changes good to merge?

@23Skidoo
Copy link
Member

@JoshMeredith Yes, feel free to push the big green button!

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.

4 participants