Skip to content

Conversation

jschaul
Copy link
Member

@jschaul jschaul commented May 12, 2022

This change leads to the following new behaviour:

  • show package name for the unit tests that run
  • stop executing other packages' unit tests if there's a failure

doesn't warrant a changelog I think.

@jschaul jschaul requested review from pcapriotti and smatting May 12, 2022 17:52
@jschaul jschaul temporarily deployed to cachix May 12, 2022 17:52 Inactive
xargs -n 1 "$DIR/cabal-run-tests.sh"
xargs -n 1 echo)

for p in $packages; do
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the workaround[1] isn't worth it, but this will cause problems if there's ever an IFS character (e.g. SPC) in a package name.

[1] Have xargs invoke a /bin/sh subshell and use $1 there instead of $p.

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 using xargs makes it not possible to stop the execution on first test failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can definately stop a GNU xargs run:

If any invocation of the command exits with a status of 255, xargs will stop immediately without reading any further input.

and that's standard behavior, too:

This sequence shall be repeated until one of the following occurs: [...] An invocation of a constructed command line returns an exit status of 255.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get the discussion; haskell package names never contain spaces, so this is a silly objection. But if that's what it takes to get a positive review on this PR that has been hanging for two weeks; then; sure, see 5e3333e.

@jschaul jschaul temporarily deployed to cachix May 23, 2022 23:15 Inactive
@jschaul jschaul temporarily deployed to cachix May 23, 2022 23:21 Inactive
Copy link
Contributor

@stephen-smith stephen-smith left a comment

Choose a reason for hiding this comment

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

Sorry, it was not my intent to hold things up or require any changes.

@jschaul jschaul merged commit c9cc20d into develop May 24, 2022
@jschaul jschaul deleted the cabal-run-all-tests-show-package branch May 24, 2022 08:54
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