Health: Break 'page.spec.js' to smaller files #2218
Conversation
yanivefraim
changed the title
test/browser.spec.js
Outdated
| */ | ||
|
|
||
| module.exports.addTests = function({ | ||
| describe, xdescribe, fdescribe, it, fit, xit, beforeAll, beforeEach, afterAll, afterEach |
There was a problem hiding this comment.
nit: let's pass testrunner here and extract the DSL below. I think it'll look better.
test/coverage.spec.js
Outdated
| describe, xdescribe, fdescribe, it, fit, xit, beforeAll, beforeEach, afterAll, afterEach | ||
| }, expect, defaultBrowserOptions, puppeteer, PROJECT_ROOT) { | ||
|
|
||
| if (process.env.COVERAGE) { |
There was a problem hiding this comment.
I'd move this into the test.js; it's an infrastructure thing and tightly depends on helper.recordPublicAPICoverage() call.
test/page.spec.js
Outdated
| describe, xdescribe, fdescribe, it, fit, xit, beforeAll, beforeEach, afterAll, afterEach | ||
| }, | ||
| expect, | ||
| defaultBrowserOptions, |
There was a problem hiding this comment.
I think defaultBrowserOptions and PROJECT_ROOT are not needed for page subtests? Not sure what's the use of puppeteer as well.
There was a problem hiding this comment.
we do use some of them. I removed unnecessary params...
test/test-driver.js
Outdated
| * @param {number=} eventCount | ||
| * @return {!Promise<!Object>} | ||
| */ | ||
| module.exports.waitForEvents = function(emitter, eventName, eventCount = 1) { |
There was a problem hiding this comment.
let's call this file utils.js and merge with frame-utils.js.
There was a problem hiding this comment.
done. Side note: I like the name driver better than utils 😃
There was a problem hiding this comment.
Thanks. driver, and especially test-driver, raises very strong associations in my head with webdriver and chromedriver
There was a problem hiding this comment.
This is a long discussion 😄
We don't like the name utils, it reminds us of a place you put stuff that you don't know what to do with. On the other hand, we use drivers to put common test stuff + encapsulate test logic / selectors. This way our tests are much more readable (they are written in "english") + they are not aware of selectors / dom implementation. If we change dom implementation, we will change selectors only in one place (and not in all tests). WDYT?
|
@aslushnikov - thanks for the review! All comments fixed. BTW - |
test/utils.js
Outdated
| @@ -1,5 +1,5 @@ | |||
| /** | |||
| * Copyright 2017 Google Inc. All rights reserved. | |||
| * Copyright 2018 Google Inc. All rights reserved. | |||
There was a problem hiding this comment.
fixed back to 2017
test/test-driver.js
Outdated
| * @param {number=} eventCount | ||
| * @return {!Promise<!Object>} | ||
| */ | ||
| module.exports.waitForEvents = function(emitter, eventName, eventCount = 1) { |
There was a problem hiding this comment.
Thanks. driver, and especially test-driver, raises very strong associations in my head with webdriver and chromedriver
@yanivefraim These look nice
Definitely. |
Started breaking page.spec.js to some small files, by subject.