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

"Empty" test files are ok on node@5 and not ok on node@4 #604

Closed
MoOx opened this issue Mar 5, 2016 · 28 comments
Closed

"Empty" test files are ok on node@5 and not ok on node@4 #604

MoOx opened this issue Mar 5, 2016 · 28 comments
Labels
bug current functionality does not work as desired priority

Comments

@MoOx
Copy link

MoOx commented Mar 5, 2016

I currently have a project with "empty" test files like this

import test from "ava"

// files added to report accurate coverage
import "../index.js"

test("todo", () => {
  console.log("TODO: test src/PageContainer/index*")
})

By empty I mean that there is not a single assertion.

This is a workaround for nyc --all (see istanbuljs/nyc#181) to get a real coverage percentage with a project that is not fully tested (yet).

Checkout this build https://travis-ci.org/MoOx/statinamic/builds/113959817. It's working on node@5 and failing on node@4 because empty test files report " Test results were not received from ...".

I expect that AVA behave the same on node@5 and node@4. So fails on both, or pass on both.

Maybe I am doing something wrong?

@MoOx
Copy link
Author

MoOx commented Mar 5, 2016

Oh. Totally sorry, I posted this issue too quickly. It's not especially related to "empty" files.
Well it seems AVA have a more global issue with node@4 (at least on Travis, cause I cannot even reproduce my problem locally)...

Will try to investigate.

@MoOx MoOx closed this as completed Mar 5, 2016
@jamestalmage
Copy link
Contributor

What version of npm are you using on node 4 locally?

Travis node 4 uses npm@2. If you've upgraded your local node 4 to use npm@3 it could be nom that is responsible for the weirdness.

AVA should behave the same across environments, so I feel this is valid.

@jamestalmage jamestalmage reopened this Mar 6, 2016
@MoOx
Copy link
Author

MoOx commented Mar 6, 2016

Latest default node@4 (nvm install 4 && nvm use 4).
I did something stupid: I just used nvm to run tests using node@4 and didn't clean node_modules. So npm@2 but with a node_modules from npm@3. Will try with a clean npm@2 install.

@ben-eb
Copy link
Contributor

ben-eb commented Mar 7, 2016

I'm getting this error too but on 0.12.

postcss/postcss-selector-parser#44

@thangngoc89
Copy link

I edited those empty test file to use test.todo and AVA 0.13.0 , same old error

https://travis-ci.org/MoOx/statinamic/jobs/115137586#L794-L800

@MoOx
Copy link
Author

MoOx commented Mar 10, 2016

Btw, if you look correctly, it's not empty files that report failure (eg: src/configurator/tests/index.js do have tests).

@thangngoc89
Copy link

Those failure files contain : sync, async, and todo test. So I assume this happens randomly

@novemberborn
Copy link
Member

@MoOx @ben-eb @thangngoc89 could you try with master? I have a feeling 5435820 may be a relevant fix.

@thangngoc89
Copy link

@novemberborn thanks. I'm setting up a branch for this.

@ben-eb
Copy link
Contributor

ben-eb commented Mar 20, 2016

@thangngoc89
Copy link

@novemberborn Sorry for the delay. We haven't run tests on node 4 for a while so there is some issues I had to be fixed first.

But like @ben-eb said. it's still failing https://travis-ci.org/MoOx/statinamic/jobs/117313475#L889-L902

@novemberborn
Copy link
Member

@ben-eb @thangngoc89 yes sorry I wasn't clear before. I was hoping the failures would at least be more consistent now.

I'll try and have a closer look at this tomorrow.

@thangngoc89
Copy link

@novemberborn this bug is now appear on node 5 too.

https://travis-ci.org/MoOx/statinamic/builds/117380398

I started to think that this is a concurrency problem since I can't re-produce this locally. Can we limit the concurrency here ?

@MoOx
Copy link
Author

MoOx commented Mar 21, 2016

Not sure why but we currently are having issue with travis/push but not travis/pr, this is realllly weird...

https://travis-ci.org/MoOx/statinamic/builds/117449079 vs https://travis-ci.org/MoOx/statinamic/builds/117449085

And this have nothing to do with empty file, since we are now using ava.todo()

@novemberborn
Copy link
Member

I started to think that this is a concurrency problem since I can't re-produce this locally. Can we limit the concurrency here ?

Yea that's my thinking too. Both the test suites have a large number of test files.

Normally AVA will fail a test run if the child process exits with a non-zero exit code, but that's not happening here. When I reduce the memory the child processes exit with a null code and a SIGABRT signal. Using https://github.com/postcss/postcss-selector-parser/:

$ node --max-executable-size=10 --max-old-space-size=15 --max-semi-space-size=1 node_modules/.bin/ava --verbose 'src/__tests__/*.js'

There's a bunch more output that's not in the CI logs though:

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory

<--- Last few GCs --->

    3515 ms: Mark-sweep 12.6 (50.1) -> 12.6 (50.1) MB, 17.9 / 0 ms [allocation failure] [GC in old space requested].
    3611 ms: Mark-sweep 12.6 (50.1) -> 12.6 (50.1) MB, 95.7 / 0 ms [allocation failure] [GC in old space requested].
    3625 ms: Mark-sweep 12.6 (50.1) -> 12.6 (50.1) MB, 14.3 / 0 ms [last resort gc].
    3642 ms: Mark-sweep 12.6 (50.1) -> 12.6 (50.1) MB, 16.4 / 0 ms [last resort gc].


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x896904e3ac1 <JS Object>
    1: parse [native json.js:44] [pc=0x282455d13288] (this=0x896904da0c1 <a JSON with map 0x20a4c2751a49>,s=0x6758b0baa81 <an Uint8Array with map 0x20a4c2705911>,m=0x89690404189 <undefined>)
    2: arguments adaptor frame: 1->2
    3: load [/private/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T/tmp.WOamj6WB/postcss-selector-parser/node_modules/babel-register/lib/cache.js:64] [pc=0x282455ddede9...

Could you try with https://github.com/sindresorhus/ava/tree/sigabrt? It'll log the SIGABRT exit reason and also include the exit signal in the "no results received" message.

@thangngoc89
Copy link

I ran the above command locally and got the process out of memory (node 5)

> node --max-executable-size=10 --max-old-space-size=15 --max-semi-space-size=1 node_modules/.bin/ava --verbose



<--- Last few GCs --->

    3668 ms: Scavenge 13.0 (50.1) -> 13.0 (50.1) MB, 0.2 / 0 ms (+ 37.8 ms in 1 steps since last GC) [allocation failure] [incremental marking delaying mark-sweep].
    3748 ms: Mark-sweep 13.0 (50.1) -> 12.9 (50.1) MB, 80.5 / 0 ms (+ 56.0 ms in 3 steps since start of marking, biggest step 37.8 ms) [last resort gc].
    3815 ms: Mark-sweep 12.9 (50.1) -> 12.9 (50.1) MB, 66.4 / 0 ms [last resort gc].


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x1fb24dce3ac1 <JS Object>
    2: /* anonymous */(aka /* anonymous */) [vm.js:~52] [pc=0x1d536f0b544] (this=0x1fb24dc04189 <undefined>,code=0x22fc581fdc11 <Very long string[4449]>,options=0x11a5c5d57391 <an Object with map 0x1ba787511049>)
    3: _compile [module.js:387] [pc=0x1d536f933d3] (this=0x11a5c5d57401 <a Module with map 0x2feb67f170e1>,content=0x3b936c004101 <Very long string[4383]>,filename=0x11a5c5d573d9 <Stri...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - process out of memory
Aborted (core dumped)

This is CI build with AVA from sigabrt branch

https://travis-ci.org/MoOx/statinamic/jobs/117540704#L889

@ben-eb
Copy link
Contributor

ben-eb commented Mar 21, 2016

@novemberborn
Copy link
Member

SIGKILL, huh. Will see if Travis has limits or if there's a circumstance in which Linux kills procs like that. Anyway looks like this is a new scenario AVA should log for (besides not failing like this).

@jamestalmage
Copy link
Contributor

Looks like there is a limit on the number of processes Travis will handle

My build script is killed without any error
...
[potiential cause might be:] Tests running in parallel using too many processes or threads (e.g. using the parallel_test gem)
...
For parallel processes running at the same time, try to reduce the number. More than two to four processes should be fine, beyond that, resources are likely to be exhausted

Seems really likely this is the issue. It seems your test suites are able to handle a lot more than just four processes though. That document is targeted towards Ruby, so maybe Node processes are lighter weight and Travis can handle more.

Throttling concurrency in AVA is likely to be non-trivial. The only solution I can think of for now is to batch via the cli:

ava test/subdir1/*.js
ava test/subdir2/*.js
ava test/subdir3/*.js

I get that that is really ugly. We will make fixing this a priority.

@jamestalmage jamestalmage added bug current functionality does not work as desired priority labels Mar 21, 2016
@novemberborn
Copy link
Member

@thangngoc89

I ran the above command locally and got the process out of memory (node 5)

Yea, the commands reduce the memory until that occurs. Big takeaway is that we're not catching these kinds of crashes, resulting in misleading log output.

I'll do a PR soon to improve AVAs output in this case. Let's discuss the forking related issues in #78. Thanks all for pushing AVA to its limits!

@ben-eb
Copy link
Contributor

ben-eb commented Mar 23, 2016

Do you have a good solution for running tests in batches with global code coverage using nyc? As far as I'm aware, running in batches will leave lines uncovered which would be hit in other test files...

@novemberborn
Copy link
Member

@ben-eb I guess you could write a wrapper script which launches a child process for each batch. nyc should be able to instrument that.

Running your tests in serial may also help, assuming Travis only kills the processes once they start executing the tests, not while they're first launched.

@jamestalmage
Copy link
Contributor

I think processes are getting killed before any tests start. The logs seem to show all the kill messages right away, then test results.

tusharmath added a commit to tusharmath/Multi-threaded-downloader that referenced this issue Mar 24, 2016
tusharmath added a commit to tusharmath/Multi-threaded-downloader that referenced this issue Mar 24, 2016
as of now its impossible to run coverage with AVA. The build keeps failing

look avajs/ava#604 (comment)
tusharmath added a commit to tusharmath/Multi-threaded-downloader that referenced this issue Mar 24, 2016
as of now its impossible to run coverage with AVA. The build keeps failing

look avajs/ava#604 (comment)
@ben-eb
Copy link
Contributor

ben-eb commented Mar 24, 2016

Just FYI, this script I wrote should be able to fix anyone's builds that are running into this error. It allows you to run AVA processes on each of your files serially, and supports code coverage. Note that it is slow, especially compared to running AVA on your local machine, so I recommend using an alternate script when not using Travis CI.

postcss/postcss-selector-parser@081a536

@thangngoc89
Copy link

@ben-eb can I steal your script ?

@ben-eb
Copy link
Contributor

ben-eb commented Mar 24, 2016

Sorry if it wasn't clear, but yes anyone is free to take this script and use it under the terms of the MIT license.

@thangngoc89
Copy link

I improved @ben-eb a little bit. You can now choose how many main AVA processes you want to spawn. They will be run concurrency. https://github.com/MoOx/statinamic/blob/3eaf91f6d58fb76eab8236eebbd5b69625182076/scripts/ava-serial.js

Does it looks like a concurrency implementation of AVA ? @jamestalmage @novemberborn

@jamestalmage
Copy link
Contributor

@thangngoc89 - There are added inefficiencies (and reduced complexities) by not implementing in AVA.

By all means - use this script until we "fix it the right way".

blond pushed a commit to bem-sdk-archive/bem-deps that referenced this issue May 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug current functionality does not work as desired priority
Projects
None yet
Development

No branches or pull requests

6 participants
@novemberborn @MoOx @ben-eb @thangngoc89 @jamestalmage and others