Skip to content

Migrate integration tests to shell based format, fixed travis #2868

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

Merged
merged 3 commits into from
Oct 15, 2015

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Oct 13, 2015

Improves on #2864

@phadej
Copy link
Collaborator Author

phadej commented Oct 13, 2015

ping @BardurArantsson @23Skidoo

@23Skidoo
Copy link
Member

@phadej Does it also work on Windows (with msys2)?

/cc @grayjay

@BardurArantsson
Copy link
Collaborator

I'm 99% percent sure it won't run on Windows, I believe a previous iteration did this exact thing.

@phadej Thanks for the fixes. I'll rework the "/bin/bash" one (see comment there) and see if I can improve on the "extra-source-files" bit sometime during the next week or so (again, see comment there).

@phadej
Copy link
Collaborator Author

phadej commented Oct 14, 2015

ie10 - win8

The only test failing on windows.

@phadej
Copy link
Collaborator Author

phadej commented Oct 14, 2015

Hmm, it was easy after all:

win8-works

@phadej
Copy link
Collaborator Author

phadej commented Oct 14, 2015

For extra-source-files, IIRC recent enough Cabal supports globs? can we use them in Cabal itself?

@phadej
Copy link
Collaborator Author

phadej commented Oct 14, 2015

And if someone wants to test on windows (now or later): https://dev.modern.ie/tools/vms is handy (minghc works nice too).

@phadej
Copy link
Collaborator Author

phadej commented Oct 14, 2015

And last comments:

  • for OSX you might need to cabal updatewith just builded cabal once to get 00-index.tar into the right place (not the place Haskell Platform puts it) - alternatively tell where HP placed it to configure script. This is very minor, won't do anything to fix this.
  • stack test doesn't work still (package-tests do not work with stack #2711):
 cabal: Use of GHC's environment variable GHC_PACKAGE_PATH is incompatible with
Cabal.

this LGTM otherwise. The extra-source-files cleanup could be done later.

I need this PR in to get going with parsec parser (there is something weird happening in my branch, and this version of integration tests is much much more nicer to debug 👍).

@BardurArantsson
Copy link
Collaborator

Yeah, I agree that the glob vs. extra-sources thing isn't a show-stopper, it can be fixed in a diferent PR. (I'll create the issue after this PR lands. If I forget, please somebody remind me.)

I'm not sure what to make of #2711, but given that it's a previously existing issue and I don't think this necessarily changes anything much about a possible resolution, I think we should just punt on fixing #2711 with this PR -- and get this merged ASAP so everybody else who wants to use EasyTests(TM) becomes unblocked.

I'll go squash into a series of meaningful commits which build individually a little later tonight (or tomorrow). At least, I think it would be best to have that, I don't know if there's a project policy for this in Cabal/cabal-install land?

@phadej : I'm not sure if I can preserve authorship of all your patches if we want each individual commit to build -- do you mind if I just pull the "source" -> "." and "extra-sources" ones into the main commit here? (I think the Windows-compat bits can be separate, but if you don't mind I'd also be happy to just pull them in.)

@23Skidoo
Copy link
Member

Globs of form foo/*.bar are supported, but recursive globs are not (yet).

@BardurArantsson
Copy link
Collaborator

Re: the glob thing. I think the only downside here that it's yet-another-surprise for when building on travis suddenly fails rather inexplicably, but perhaps we should just have a FAQ-type thing? (Or maybe a "checklist-before-creating-a-PR"?). Obviously, it would be ideal to just have an "extra-source-dir" or full recursive glob support, but until then this really only affects the PR phase of development. During normal running-of-tests, it's completely irrelevant.

@@ -0,0 +1,276 @@
-- | Groups black-box tests of cabal-install and configures them to test
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the tasty-golden package can be used to simplify code in this file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did check out tasty-golden, but it doesn't really seem to get us much since all the traversal stuff it pretty specialized anyway. Did you have something particular in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, it just looked like tasty-golden does something similar. I haven't read the code closely.

@phadej
Copy link
Collaborator Author

phadej commented Oct 14, 2015

@BardurArantsson, I'm not particularly attached to those commits, thet are quite mechanical anyway. Actually I'll just squash everything myself.

This is required for the reworking of integration tests
to use shell scripts.
Fixes haskell#2797

Squashed commits:
- Use ., not source
@phadej phadej force-pushed the gh-2797-2 branch 2 times, most recently from 122c330 to 1919061 Compare October 14, 2015 17:15
- findExecutable sh
- Use single quotes for cabal exec
@BardurArantsson
Copy link
Collaborator

@phadej Great, thanks. AFAICT this is good to go from my perspective.

@@ -0,0 +1,6 @@
. ../common.sh

# We should probably be using a .err file and should_fail,
Copy link
Member

Choose a reason for hiding this comment

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

One possible future solution for these kinds of problems is to add support for platform-specific .err files.

Copy link
Member

Choose a reason for hiding this comment

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

Or we could add support for matching against a set of regexes.

Copy link
Member

Choose a reason for hiding this comment

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

Also see how LLVM solves this: http://llvm.org/docs/CommandGuide/FileCheck.html

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I considered line-by-line regex comparison instead, but it's a bit noisy if you have to escape all the lines containing special characters (and where you don't want special treatment). FileCheck looks somewhat similar to that, but I think it's probably overkill for what we want.

Maybe we could add something simple here like line-based comparison, but with

   RE:this is a regex
   this is not

i.e. just using the a RE: prefix to designate regexes. (It's ulikely enough to appear in actual output that it wouldn't need to be "escapable".)

Copy link
Member

Choose a reason for hiding this comment

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

The RE: prefix idea sounds good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll create an issue to add it and assign it to myself. (No guarantees on when I'll get 'round to it, though.)

@23Skidoo
Copy link
Member

OK, I think this is good to go.

@phadej @BardurArantsson Thanks for your contributions!

23Skidoo added a commit that referenced this pull request Oct 15, 2015
Migrate integration tests to shell based format, fixed travis
@23Skidoo 23Skidoo merged commit 41d8906 into haskell:master Oct 15, 2015
@BardurArantsson
Copy link
Collaborator

... and thanks to @ezyang and @grayjay for early review, testing and feedback!

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.

3 participants