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: add test/addons to default test list #314

Closed
wants to merge 4 commits into from

Conversation

bnoordhuis
Copy link
Member

R=@rvagg, /cc @othiym23

The only not-so-nice thing is that make test-all rebuilds the addons every time you run it. I'll try to figure out something for that later.

@rvagg
Copy link
Member

rvagg commented Jan 12, 2015

I'm ok with rebuilding every time you run as compiling is part of the addon process and worth testing--although if you can come up with a solution that prevents the compile if nothing has changed then +1.

Is your recommendation that CI should run make test-all, I've avoided that because it's just so big and takes so long.

@bnoordhuis
Copy link
Member Author

@rvagg make test-all would be best but yes, it takes a godawful long time. I can add a test-most target that drops the pummel and gc tests and maybe the internet tests.

@piscisaureus mentioned he would like to have the internet tests run as part of the build but they can be pretty flaky so I don't know.

@rvagg
Copy link
Member

rvagg commented Jan 13, 2015

@bnoordhuis how about adding a test-ci target that's explicitly for CI purposes so it can be discussed and adjusted in this repo

`make test-all` and `python tools/test.py` will now also run the addon
tests in test/addons.
The test-npm target builds documentation, changes versioned files,
clutters the current working directory with artifacts, etc.  In short,
it doesn't seem quite ready for inclusion in `make test-all`.
Add a test-ci target that is like test-all minus the (slow) pummel and
gc test suites.

This is primarily intended for the continuous integration, where we want
decent coverage but don't want to wait for ages for tests to complete.
Commit 3e1b1dd ("Remove excessive copyright/license boilerplate") trips
up the copyright boilerplate style check.  Disable it.
@bnoordhuis
Copy link
Member Author

@rvagg Added, PTAL.

What do we do with the test-npm target? @othiym23's change in f468e5f makes it much more palatable but it's still very slow and (I think) it requires an internet connection and (presumably) a working npmjs.org. I can drop the commit that removes it from test-all but do you think it's a good idea to add to test-ci?

@othiym23 Is there a meaningful subset that we can run that does not depend on external resources?

@othiym23
Copy link
Contributor

@bnoordhuis Right now, not really, due to various regression tests depending on gists and resources on the registry. Sometime in Q2 / Q1 we're going to rewrite the test suites to be more sane, and part of that will be splitting up unit and integration / functional tests. I think we can also commit to making it only needing a localhost for its networking tests at that point. But that doesn't really help you now, for which I apologize.

bnoordhuis added a commit that referenced this pull request Jan 13, 2015
test: add test/addons to default test list

`make test-all` and `python tools/test.py` will now also run the addon
tests in test/addons.

test: remove test-npm from test-all make target

The test-npm target builds documentation, changes versioned files,
clutters the current working directory with artifacts, etc.  In short,
it doesn't seem quite ready for inclusion in `make test-all`.

test: add test-ci target, reduced test-all

Add a test-ci target that is like test-all minus the (slow) pummel and
gc test suites.

This is primarily intended for the continuous integration, where we want
decent coverage but don't want to wait for ages for tests to complete.

cpplint: add -license/copyright to default filters

Commit 3e1b1dd ("Remove excessive copyright/license boilerplate") trips
up the copyright boilerplate style check.  Disable it.

PR-URL: #314
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Jan 13, 2015

landed in 7861ff4 as is

I'd like to put test-npm in to test-ci but it's failing for me currently so I don't know how long it even takes, will punt on this for later but ideally CI will touch npm

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