Tests could be run in random order by phadej · Pull Request #1382 · purescript/purescript

phadej · 2015-08-14T03:51:35Z

No description provided.

paf31 · 2015-08-14T03:55:14Z

What does this gain us? I'm not sure I follow what's going on here.

phadej · 2015-08-14T04:07:36Z

I tried whether stackage could build tests (i.e. tests pass when package fetched from hackage). Seems they do, except for some reason it run tests in tests-psci tests order, so the first one failed.

EDIT: I'll also try to run tests from sdisted package. WIP.

phadej · 2015-08-14T07:34:41Z

.travis.yml

compiler hack creates distinct caches for each entry.

phadej · 2015-08-14T08:24:19Z

Ok, now I'm done. Seems to work, and caching is more robust.

I'll make yet another PR to separate test running from sdist package in separate job.

Sorry for a lot of travis tweaking and PR noise, but I just nerdsnipped myself into the CI stuff, and purescript is complex enough project to be interesting to tweak.

garyb · 2015-08-14T09:52:03Z

This seems to break on Windows due to tests.exe: npm: callProcess: does not exist - we had a similar issue in psc-publish, I think on mingw32 you'll need to look for npm.cmd for it to work. (See #1320)

phadej · 2015-08-14T09:54:21Z

I guess this doesn't change windows behaviour, i.e. it doesn't work in master either. I unfortunately don't have windows machine available to me right now to test on.

garyb · 2015-08-14T09:57:27Z

Yeah, it doesn't work in master for a different reason currently (it fails in a different way I mean), that's why I tried this out in hopes of having it work. 😄

If you can add the #1320 style executable finding I'd be more than happy to test it again.

phadej · 2015-08-14T10:09:15Z

Yeah, tests already search for node and nodejs.

garyb · 2015-08-14T10:10:44Z

Those are fine as it is node.exe on Windows (which works with the normal method of finding executables). It's specifically npm and things installed by npm that need to have the .cmd extension - the failure was due to some of the psci tests, so it might be there's a different problem entirely.

phadej · 2015-08-14T10:11:21Z

tests/common/TestsSetup.hs

This could be generalised and moved somewhere into shared code, @garyb opinions? seems quite the same as in psc-publish

phadej · 2015-08-14T10:16:59Z

I meant something like

findFirstExecutable :: [String] -> IO (Maybe String)
findFirstExecutble = ...

findNode = findFirstExecutable ["node", "nodejs"]
findBower = findFirstExecutable ["bower", "bower.cmd"]

IMHO it's not dangerous to look for bower.cmd on unix systems too.

garyb · 2015-08-14T12:55:18Z

That would make sense. I don't feel strongly about whether we should always search for bower.cmd as a fallback or not, but @michaelficarra was against it.

paf31 · 2015-08-14T14:14:17Z

tests/common/TestsSetup.hs

For code which is licensed with a different copyright ascription, we've started requiring the license to be explicitly listed or linked here, to make it clear. Could you add a link to the MIT license if that's the one you're using?

Thanks.

For example:

purescript/src/Language/PureScript/Sugar/TypeClasses/Deriving.hs

Lines 4 to 5 in 0ec9d41

-- Copyright : (c) Gershom Bazerman 2015

-- License : MIT (http://opensource.org/licenses/MIT)

Actually, I just noticed this isn't new code, it's just been extracted verbatim. Since this code isn't written by me (or @garyb), I'm not at liberty to transfer copyright of it. If it's going to be relicensed back to the project under different copyright assumptions, it needs to be done under the terms of the original license, which means including that here. I think I'm going to put something in CONTRIBUTING.md about how copyright notices should be added, but I think at a minimum, the code must be new. IANAL though, etc. I hate to be pedantic about these things, but as I say, this isn't my code.

I've written up the new approach, which is hopefully as simple as possible, here:
#1394

phadej · 2015-08-17T04:57:06Z

@garyb, Do I need to do a windows fix in this branch, or can this be merged? I'll be a bit more busy this week unfortunately.

garyb · 2015-08-17T10:29:08Z

I'm happy for it to go in as-is since it's not newly breaking anything anyway. I can fix the Windows specific stuff afterwards. :)

phadej · 2015-08-18T11:25:44Z

ping @paf31 ?

paf31 · 2015-08-18T15:57:27Z

I need to understand the code changes still, but first, slightly less pleasant business:

Copyright notices: New files which are entirely original can have copyright notices added for their author, but the file you added is largely copied verbatim from elsewhere. If it were my code, I probably wouldn't care, but none of that code was written by me, so the copyright belongs with the original authors and is licensed to the project users. Claiming copyright over the whole file would be incorrect. So there are two options: a) leave the copyright notice as it was, with copyright assigned to your code implicitly by CONTRIBUTORS.md, or b) relicense the existing code under the terms of the MIT license (or a compatible license) using the existing code, as a derived work.

Sorry to make a deal about it. Perhaps the original authors don't care about this honestly, but it's the first time I've had to think about things like this, and I feel it's important to be crystal clear about code ownership in a large(ish) code base.

phadej · 2015-08-18T19:31:49Z

The file functions are copied from are Copyright : (c) Phil Freeman 2013, so that file could have copyright by you.

For me (as a conributor) would be easier to have Copyright : (c) PureScript compiler project contributors everywhere, after all now we have CONTRIBUTORS.md and vcs history to track who did what (and when).

EDIT: actially e.g. ASF doesn't even have copyright notices in the files, only license information: http://www.apache.org/legal/src-headers.html - that makes much sense in multi-contributor projects

paf31 · 2015-08-18T19:45:56Z

Yes, I plan to remove those headers once all the contributors have their entries in CONTRIBUTORS.md. You could leave it off entirely if you like. The LICENSE file would cover it.

phadej · 2015-08-18T19:54:49Z

Copyright notice removed

paf31 · 2015-08-18T20:07:34Z

.travis.yml

Should be psci-tests?

paf31 · 2015-08-18T20:21:35Z

.travis.yml

Won't this share the sandbox between the main build and the sdist test? @hdgarrood Is that ok?

@paf31 no, the compiler trick in build matrix does the job. In travis-ci/travis-ci#4393 is an issue, and this hackish - workaround mentioned.

Which sandbox does test-install.sh use then? It says cabal sandbox init --sandbox ../.cabal-sandbox below.

The same, but the sandbox is snapshotted away (to the cache location) before we install purescript into it. Thus we don't need to install dependencies twice, ever.

EDIT sorry, didn't understood your original question

test-install is there to verify that everything that needs to be exported is listed in the .cabal file, so I think this is fine. 👍

phadej · 2015-08-18T20:49:04Z

.travis.yml

@paf31 here we move sandbox contents to the cached location

garyb · 2015-08-22T19:44:08Z

What's happening with this? I'd like to get it in so I can update and merge #1402 :)

Although since this already needs rebasing, perhaps I should merge that anyway?

phadej · 2015-08-22T20:38:57Z

I'll rebase

Also refactor .travis.yml based on ideas from https://github.com/hvr/lzma-streams/blob/648be0a44b7e454393c34d542a5467c44124413d/.travis.yml

(to trigger new build, to check cache behaviour)

phadej · 2015-08-22T20:58:29Z

Rebased, waiting only for non-cache lts-3.1 build to complete (cache works!)

paf31 · 2015-08-23T02:14:24Z

Yes, I'm going to merge this before I release 0.7.4.

Tests could be run in random order

paf31 · 2015-08-23T21:44:32Z

Thank you very much!

phadej force-pushed the test-random-order branch 2 times, most recently from a1082e5 to 2386068 Compare August 14, 2015 07:32

phadej reviewed Aug 14, 2015
View reviewed changes

.travis.yml

Copy link
Copy Markdown

Contributor Author

phadej Aug 14, 2015

Choose a reason for hiding this comment

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

compiler hack creates distinct caches for each entry.

phadej reviewed Aug 14, 2015
View reviewed changes

paf31 reviewed Aug 14, 2015
View reviewed changes

phadej force-pushed the test-random-order branch from e4a69ef to 49de8b3 Compare August 18, 2015 19:54

paf31 reviewed Aug 18, 2015
View reviewed changes

phadej force-pushed the test-random-order branch from 49de8b3 to dfd4738 Compare August 18, 2015 20:13

paf31 reviewed Aug 18, 2015
View reviewed changes

garyb mentioned this pull request Aug 18, 2015

Fixes running tests on Windows #1402

Merged

phadej reviewed Aug 18, 2015
View reviewed changes

.travis.yml

Copy link
Copy Markdown

Contributor Author

phadej Aug 18, 2015

Choose a reason for hiding this comment

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

@paf31 here we move sandbox contents to the cached location

phadej added 5 commits August 22, 2015 23:40

Tests could be run in random order

72359d7

Test sdist installation

beb0556

Run sdist tests

ae731b3

Also refactor .travis.yml based on ideas from https://github.com/hvr/lzma-streams/blob/648be0a44b7e454393c34d542a5467c44124413d/.travis.yml

Reuse sandbox in test-install.sh

66e78d8

Add comment in .travis.yml

fc8cea1

(to trigger new build, to check cache behaviour)

phadej force-pushed the test-random-order branch from dfd4738 to fc8cea1 Compare August 22, 2015 20:41

paf31 added a commit that referenced this pull request Aug 23, 2015

Merge pull request #1382 from phadej/test-random-order

1c78f66

Tests could be run in random order

paf31 merged commit 1c78f66 into purescript:master Aug 23, 2015

phadej deleted the test-random-order branch September 14, 2015 06:11

	-- Copyright : (c) Gershom Bazerman 2015
	-- License : MIT (http://opensource.org/licenses/MIT)

Conversation

phadej commented Aug 14, 2015

Uh oh!

paf31 commented Aug 14, 2015

Uh oh!

phadej commented Aug 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phadej commented Aug 14, 2015

Uh oh!

garyb commented Aug 14, 2015

Uh oh!

phadej commented Aug 14, 2015

Uh oh!

garyb commented Aug 14, 2015

Uh oh!

phadej commented Aug 14, 2015

Uh oh!

garyb commented Aug 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phadej commented Aug 14, 2015

Uh oh!

garyb commented Aug 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phadej commented Aug 17, 2015

Uh oh!

garyb commented Aug 17, 2015

Uh oh!

phadej commented Aug 18, 2015

Uh oh!

paf31 commented Aug 18, 2015

Uh oh!

phadej commented Aug 18, 2015

Uh oh!

paf31 commented Aug 18, 2015

Uh oh!

phadej commented Aug 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyb commented Aug 22, 2015

Uh oh!

phadej commented Aug 22, 2015

Uh oh!

phadej commented Aug 22, 2015

Uh oh!

paf31 commented Aug 23, 2015

Uh oh!

paf31 commented Aug 23, 2015

Uh oh!

Reviewers

Assignees

Labels