Implemented TestCafe configuration file#3147
Conversation
|
❌ Tests for the commit a9cdfb4 have failed. See details: |
|
❌ Tests for the commit 9852442 have failed. See details: |
|
❌ Tests for the commit 0123540 have failed. See details: |
|
❌ Tests for the commit 359550f have failed. See details: |
|
❌ Tests for the commit 8983974 have failed. See details: |
|
❌ Tests for the commit b0560d4 have failed. See details: |
|
❌ Tests for the commit 48ef014 have failed. See details: |
|
@testcafe-build-bot retest |
|
❌ Tests for the commit 48ef014 have failed. See details: |
|
❌ Tests for the commit 48ef014 have failed. See details: |
|
❌ Tests for the commit 6d6fbee have failed. See details: |
|
❌ Tests for the commit bec09cc have failed. See details: |
|
❌ Tests for the commit ec4751e have failed. See details: |
|
❌ Tests for the commit 34723b6 have failed. See details: |
|
❌ Tests for the commit 520a1d6 have failed. See details: |
|
❌ Tests for the commit 7b1adce have failed. See details: |
|
❌ Tests for the commit 0192eeb have failed. See details: |
|
❌ Tests for the commit cc97061 have failed. See details: |
|
❌ Tests for the commit d42504d have failed. See details: |
|
❌ Tests for the commit 3711db2 have failed. See details: |
|
❌ Tests for the commit 2676546 have failed. See details: |
|
❌ Tests for the commit dca245b have failed. See details: |
|
❌ Tests for the commit 3e43550 have failed. See details: |
|
✅ Tests for the commit a0f190e have passed. See details: |
miherlosev
changed the title
src/runner/bootstrapper.js
Outdated
|
|
||
| _getReporterPlugins () { | ||
| async _ensureOutStream (outStream) { | ||
| if (typeof outStream === 'string') { |
There was a problem hiding this comment.
if (typeof outStream !== 'string')
return outStream;
const fullReporterOutputPath = resolvePathRelativelyCwd(outStream);
await makeDir(path.dirname(fullReporterOutputPath));
return fs.createWriteStream(fullReporterOutputPath);|
|
||
| const DEBUG_LOGGER = debug('testcafe:runner'); | ||
|
|
||
| const DEFAULT_TIMEOUT = { |
There was a problem hiding this comment.
We use the following notation
const DEFAULT_TIMEOUT = {
selector: 10000,
assertion: 3000,
pageLoad: 3000
};
src/runner/index.js
Outdated
| } | ||
|
|
||
| static _validateReporterOutput (obj) { | ||
| if (obj !== void 0 && |
There was a problem hiding this comment.
Let's refactor it:
const <varName> = ...
if(<varName>)
throw new GeneralError(...);There was a problem hiding this comment.
Also use this pattern here - https://github.com/DevExpress/testcafe/pull/3147/files#diff-4a70fd82331cb2921ccb359a82ce00f1R51
|
|
||
| browsers (...browsers) { | ||
| this.bootstrapper.browsers = this.bootstrapper.browsers.concat(flatten(browsers)); | ||
| if (this.apiMethodWasCalled.browsers) |
There was a problem hiding this comment.
Perhaps we could refactor FlagList to throw an exception in a field setter? Thus we won't need all the if(this.apiMethodWasCalled) conditions.
There was a problem hiding this comment.
I disagree with you. FlagList is a simple concept. Don't need to extend it with error generation.
| return parsedOptions; | ||
| } | ||
|
|
||
| export async function ensureOptionValue (optionName, optionValue) { |
There was a problem hiding this comment.
It had better use pure functions everywhere it's possible.
export async function ensureOptionValue (optionName, optionValue) {
const convertedValue = convertToBestFitType(optionValue);
return ensureFileOptionValue(optionName, convertedValue);
}
src/configuration/index.js
Outdated
| const CONFIGURATION_FILENAME = '.testcaferc.json'; | ||
|
|
||
| const ERR_READ_CONFIG_FILE = 'An error is occurred during reading the configuration filename.'; | ||
| const ERR_CONFIG_FILE_IS_NOT_WELLFORMATTED = 'Configuration filename is not well-formatted.'; |
There was a problem hiding this comment.
I think this message doesn't reflect that json is can't be parsed. Probably it suppose well-formed, but anyway it should describe expected format (json is not well-formed).
There was a problem hiding this comment.
Ok. In this case, I propose to use this message:
Configuration file is not well-formed.
Failed to parse '.testcaferc.json' data.
.testcaferc.json must be actual JSON.This message is similar with
There was a problem hiding this comment.
Ok, but anyway it should be checked by tech writers.
src/configuration/index.js
Outdated
|
|
||
| const CONFIGURATION_FILENAME = '.testcaferc.json'; | ||
|
|
||
| const ERR_READ_CONFIG_FILE = 'An error is occurred during reading the configuration filename.'; |
There was a problem hiding this comment.
It's a GeneralError, isn't it?
There was a problem hiding this comment.
It's an error about problem with reading configuration file. Not just reading file.
We will convert ERR_READ_CONFIG_FILE to the GeneralError when another reading file code will be appeared.
src/configuration/index.js
Outdated
| }); | ||
|
|
||
| if (overridenOptions.length) | ||
| console.log(`${overridenOptions.map(option => `"${option}"`).join(', ')} option${overridenOptions.length > 1 ? 's' : ''} from configuration file will be ignored.`); // eslint-disable-line no-console |
There was a problem hiding this comment.
We should keep all strings in constants.
There was a problem hiding this comment.
It's a calculated string. Its value depends on the overridenOptions array, declared in the function.
There was a problem hiding this comment.
We can store it in the notifications/warning-messages and then use renderWarning to render a string for output.
There was a problem hiding this comment.
Ok. Then it will be a warning message instead of the log message as we discussed earlier.
@kirovboris
There was a problem hiding this comment.
@AndreyBelym, I don't think the outputting this message as a warning is a good idea. It's just a log message.
| hostname: this.getOption('hostname'), | ||
| port1: this.getOption('port1'), | ||
| port2: this.getOption('port2'), | ||
| options: { |
There was a problem hiding this comment.
Is it ok, that eslint doens't throw error here?
hostname: this.getOption('hostname'),
port1: this.getOption('port1'),
port2: this.getOption('port2'),
options: {
ssl: this.getOption('ssl'),
developmentMode: this.getOption('developmentMode'),
retryTestPages: !!this.getOption('retryTestPages')
}There was a problem hiding this comment.
I wrote the options property without an empty line.
eslint will generate an error if I add the empty line.
There was a problem hiding this comment.
Maybe you just had to remove an extra space in options: {?
We already use such formatting:
https://github.com/DevExpress/testcafe/blob/master/test/functional/config.js#L30
src/configuration/index.js
Outdated
| } | ||
|
|
||
| try { | ||
| const optionsObj = JSON.parse(configurationFileContent); |
There was a problem hiding this comment.
It makes sense to use JSON5 rather than standard JSON: it has relatively relaxed grammar and allows code comments which can be quite handy sometimes. AFAIK many popular tools use it.
Also, there is a trend recently to use JS files that export configuration object instead of dealing with all the JSON-related shenanigans. Thus, you can have arbitrary configuration logic that depends on environment, for example.
There was a problem hiding this comment.
I glad to see you @inikulin . We've discussed about other configuration file formats (json, js and yml) and decided to support only the JSON format in the first implementation.
Thank you for the provided information about JSON5. But I think we will use current JSON implementation until there are problems with it.
There was a problem hiding this comment.
I like this idea and I have nothing to add to the mentioned pros.
@inikulin, I glad to see you too 😄.
|
❌ Tests for the commit 05c688f have failed. See details: |
|
@testcafe-build-bot retest |
|
❌ Tests for the commit 05c688f have failed. See details: |
|
✅ Tests for the commit 05c688f have passed. See details: |
|
FPR |
|
ping @kirovboris |
kirovboris
left a comment
kirovboris
left a comment
There was a problem hiding this comment.
/r-
just minor remarks
src/configuration/index.js
Outdated
| mergeOptions (options) { | ||
| const overridenOptions = []; | ||
|
|
||
| Object.entries(options).map(item => { |
There was a problem hiding this comment.
item -> ([key, value])
Here and in the other usages of Object.entries
src/configuration/init.js
Outdated
| @@ -0,0 +1,11 @@ | |||
| import Configuration from './index'; | |||
|
|
|||
| export default async function initConfiguration (options) { | |||
There was a problem hiding this comment.
It's should be just a method of Configuration class
const configuration = new Configuration(...);
await configuration.init(...);
|
✅ Tests for the commit 16d6697 have passed. See details: |
|
FPR |
* initial implementation * .filter changes * fix unit tests * ensureArray option for src and browsers * .reporter changes * tests * small refactoring * small refactoring 2 * functional tests * fix reporter tests * remove .only * fix test * test for reporters * fix test for concurrency * fix eslint * resolvePathRelativelyCwd * minor refactoring * fix test * fix remarks * fix tests * fix tests again * corrector's changes * move concurrency to ops.concurrency * fix remarks * fix remarks 2 * remove additional spaces between parameters
5 participants
No description provided.