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

Framework: introduce config file with all test names to be included in the single tests runner #3801

Merged
merged 4 commits into from
Mar 6, 2016

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Mar 6, 2016

This PR depends on PR #3797 from @blowery and follows work done by @aduth in #3773. It tries to remove some boilerplate code around requiring test files in */test/index.js file. The idea here is to have single runner that loads all test files from single JSON config file. Config file is created in a way that should allow us to remove it completely once we are done with removing Makefile files. As part of this change I also got rid of other single test runners we created earlier this week and included them in single tests runner created by @blowery:

  • state
  • lib/domains

How it works

I followed that idea of @aduth to have describe block which reflects folders structure of our client application. Basically test structure is identical to what was merged in #3773. Example:

Folder name: client/state/plugins/wporg/test/

Test files:

  • reducer.js
  • test-actions
  • test-selectors

Translates into config file:

{
    "state": {
        "plugins": {
            "wporg": {
                "test": [ "reducer", "test-actions", "test-selectors" ]
            }
        },

}

Test output:

state
    plugins
        wporg
            reducer (taken from describe inside the file)
                √ Test name
            test-actions
                √ Test name
            test-selectors
                √ Test name

Testing

  1. Go to client folder.
  2. Execute make test.

Performance

Single test runner contains at this moment 593 passing and 16 pending tests.

Execution time:

real    0m3.387s
user    0m3.116s
sys     0m0.402s

Open issues and possible improvements

  1. In the runner file we have at the moment this code:
const tests = process.argv.length > 2
    ? process.argv.slice( 2 )
    : glob.sync( '**/test/index.js', {} );

I'm not sure if it still works with changes I made. I'm leaving it as for now, because I want to see feedback first.
2. Config containing test file names reflects our existing folders structure, so it should be relatively easy to implement logic that executes only subset of tests based on path provided as input param.

@gziolo gziolo self-assigned this Mar 6, 2016
@gziolo gziolo added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 6, 2016
@gziolo gziolo changed the title Update/use client testsuite config Framework: introduce config file that stores all test names that should be included in the single tests runner Mar 6, 2016
@gziolo gziolo changed the title Framework: introduce config file that stores all test names that should be included in the single tests runner Framework: introduce config file with all test names to be included in the single tests runner Mar 6, 2016
@blowery
Copy link
Contributor

blowery commented Mar 6, 2016

I'm not sure if it still works with changes I made. I'm leaving it as for now, because I want to see feedback first.

It should, it's totally independent of testsuite.js at the moment. It's main purpose is giving Circle CI a way to specify individual tests or test suites to run on separate containers. It's mostly a proof-of-concept to show that we can run individual tests or suites, but still have the global wrapper that does some work, currently spinning up nock and injecting sinon chai into chai.

We can certainly teach it about the json config and pull in tests that way. Then we can filter the test suite kind of like the --grep param, but be more intelligent and only load paths that match our criteria.

The tricky bit here is figuring out how to split up the tests on circle ci. We don't have a lot of fancy ways to do the split,basically we can use either a file glob (that's the intent of the **/test/index.js pattern) or we can divy up the work ourselves based on the index of the container — we get passed a [0, $numCountainers) index and $numContainers. We could use the latter to split up the tests, but it presupposes that we're doing all the tests through the main runner. In the interim while we're moving things over, I think we just keep splitting up by Makefile and run all of the new-style tests in one batch.

Once we move everything over, we can revisit how we do the split. As you mentioned, at that point we might not even need the whitelist, so perhaps we can keep on using the simple file glob.

@blowery blowery removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 6, 2016
@blowery
Copy link
Contributor

blowery commented Mar 6, 2016

Ah, you targeted my branch! Excellent. Landing this.

blowery added a commit that referenced this pull request Mar 6, 2016
…config

Framework: introduce config file with all test names to be included in the single tests runner
@blowery blowery merged commit 0a9fae2 into update/lib-tests Mar 6, 2016
@blowery blowery deleted the update/use-client-testsuite-config branch March 6, 2016 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants