Skip to content

test: update travis config, handle errors properly, add TEST_LLNODE_DEBUG - #144

Merged
joyeecheung merged 5 commits into
nodejs:masterfrom
joyeecheung:log-error-timeout
Nov 30, 2017
Merged

test: update travis config, handle errors properly, add TEST_LLNODE_DEBUG#144
joyeecheung merged 5 commits into
nodejs:masterfrom
joyeecheung:log-error-timeout

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Nov 3, 2017

Copy link
Copy Markdown
Member

At the moment when a test times out (usually because the expected lldb output does not show up, possible due to an error), there is only a tape timeout message, as is what's happening in our Travis CI build.

This PR checks the timeout and throws an error containing the lldb output for the ease of debugging.

I've tried it on Mac and Linux with v8.9.0 and v6.11.5. I've noticed that the tests are a bit flaky in master, with random timeouts (with this patch on I can see the sent command got swallowed, it's 8 <command> instead of v8 <command> so lldb will error and the test times out). Sometimes there is a ECONNRESET on the pipe. I'll investigate that later.

@joyeecheung

joyeecheung commented Nov 3, 2017

Copy link
Copy Markdown
MemberAuthor

The Travis builds now error instead of timing out. There is a reproduction of the "swallowed v": https://travis-ci.org/nodejs/llnode/jobs/296685259. Others seems to stuck in v8 bt.

@joyeecheung

joyeecheung commented Nov 7, 2017

Copy link
Copy Markdown
MemberAuthor

Just noticed that since we only push the stdout lines into the buffer, the stderr lines are not available...for example on linux where process save-core is not available, the line error: Failed to save core file for process: no ObjectFile plugins were able to save a core for this process is written into stderr, not stdout.

Need to rework this a bit to show stderr lines when timing out as well.

@joyeecheungjoyeecheung changed the title test: show lldb output when timing out[wip] test: show lldb output when timing outNov 7, 2017
@joyeecheung
joyeecheungforce-pushed the log-error-timeout branch 3 times, most recently from fcf3962 to 2e4121eCompareNovember 28, 2017 15:31
@joyeecheung
joyeecheungforce-pushed the log-error-timeout branch 3 times, most recently from be87066 to 463135eCompareNovember 28, 2017 16:24
@joyeecheung
joyeecheungforce-pushed the log-error-timeout branch 2 times, most recently from 13bea62 to 4f7157cCompareNovember 28, 2017 18:08
@joyeecheungjoyeecheung changed the title [wip] test: show lldb output when timing outtest: update travis config, handle errors properly, add TEST_LLNODE_DEBUGNov 28, 2017
@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Travis is now green (see https://travis-ci.org/nodejs/llnode/builds/308579054), although sometimes it fails because of #150 .

This is now ready for review, cc @cjihrig@indutny@bnoordhuis@hhellyer , thanks!

cjihrig

This comment was marked as off-topic.

@joyeecheung

joyeecheung commented Nov 29, 2017

Copy link
Copy Markdown
MemberAuthor

@cjihrig Added a containsLine function to DRY the usage-test. The process.version comparison can be fixed in a later PR.

If there are no more reviews in 2 days I would like to land this so we can get a green Travis CI. The jenkins one probably need a bit of configuration update (last time I tried it it was using the old flow to build the plugin and failed to build it).

joyeecheung

This comment was marked as off-topic.

bnoordhuis

This comment was marked as off-topic.

This was referenced Nov 30, 2017
@joyeecheung
joyeecheung merged commit 65c577c into nodejs:masterNov 30, 2017
Sign up for freeto 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

@joyeecheung@bnoordhuis@cjihrig