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

test: test each block in addon.md contains js & cc #4411

Closed

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Dec 24, 2015

Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc (breaking all CI)

/cc @nodejs/build @jasnell @thealphanerd

Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc
@rvagg
Copy link
Member Author

rvagg commented Dec 24, 2015

@MylesBorins
Copy link
Contributor

works locally LGTM

rvagg added a commit that referenced this pull request Dec 24, 2015
Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc

PR-URL: #4411
Reviewed-By: Myles Borins <[email protected]>
@rvagg
Copy link
Member Author

rvagg commented Dec 24, 2015

CI all passed, landed with some urgency as 7c9cdb0 so we can get CI working again, please don't let that stop further review! Changes can be added to #4412.

@rvagg rvagg closed this Dec 24, 2015
@Trott
Copy link
Member

Trott commented Dec 24, 2015

Since we've now had a situation where a doc-only commit broke CI, is it worth revisiting the "no need to run doc-only PRs through CI" practice? I'd be all for a "every PR goes through CI, no exceptions" policy. Sure, it's rare and surprising when something like that breaks tests, but that's exactly the type of thing CI is there to surface.

Alternatively, we could get rid of the "test the code in the docs" business in test/addons. I'm less enthusiastic about that as I think it's valuable to run the documentation examples through some checking to make sure there aren't errors. All for making the code that handles it more robust, though.

Or we can carry on as before as this wasn't that big a deal in the grand scheme of things. I'd prefer to seize an opportunity to reduce our tolerance for glitches though.

@jbergstroem
Copy link
Member

Can we reduce the amount of stuff we'd build/test for in a doc only PR? When we introduced the merge-pr jenkins job we had options for doc-only PR's; can perhaps we can do something similar with node-test-pr?

@Trott
Copy link
Member

Trott commented Dec 24, 2015

@jbergstroem Ooh, yes, that would be great. Just run the doc-relevant stuff and maybe only on a subset of platforms.

@jbergstroem
Copy link
Member

@Trott could you create an issue in nodejs/build about it? I can do it otherwise.

@Trott
Copy link
Member

Trott commented Dec 24, 2015

@jbergstroem Done. nodejs/build#287

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc

PR-URL: nodejs#4411
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc

PR-URL: #4411
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc

PR-URL: #4411
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc

PR-URL: nodejs#4411
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc

PR-URL: nodejs#4411
Reviewed-By: Myles Borins <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Allows more freedom in adding additional headings to addon.markdown,
otherwise it'll try and convert each block under a heading to a test
case. We need to have at least a .js and a .cc in order to have
something to test.

Fixes regression caused by adding a new 3rd-level heading in
d5863bc

PR-URL: nodejs#4411
Reviewed-By: Myles Borins <[email protected]>
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