-
Notifications
You must be signed in to change notification settings - Fork 672
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
Screenshot file patterns (closes #1602, #1651, #1974, #1975) #2086
Conversation
❌ Tests for the commit 82c465f have failed. See details: |
Hi @thireven, Thank you for your contribution, we appreciate it a lot. Your proposal and current achievements look impressing!
Maybe we shouldn't overwrite screenshots in this case because it can be unexpected for a person. I guess we can just add a numeral postfix for a file name in this case. /cc @VasilyStrelyaev @DevExpress/testcafe Please take a look at this proposal, do you have some comments?
Could you please clarify what errors do you have. Meanwhile we'll try to run in on our side with the same environment. Also we have a |
src/cli/argument-parser.js
Outdated
@@ -89,6 +89,7 @@ export default class CLIArgumentParser { | |||
.option('-b, --list-browsers [provider]', 'output the aliases for local browsers or browsers available through the specified browser provider') | |||
.option('-r, --reporter <name[:outputFile][,...]>', 'specify the reporters and optionally files where reports are saved') | |||
.option('-s, --screenshots <path>', 'enable screenshot capturing and specify the path to save the screenshots to') | |||
.option('-p, --pattern <pattern>', 'screenshot files are named using specific patterns: %BROWSER%, %BROWSERVERSION%, %OS%, %OSVERSION%, %USERAGENT%, %DATE%, %TIME%, %FIXTURE%, %TEST%, %TESTNUMBER%, %FILENUMBER%') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
use patterns to compose screenshot file names and paths: %BROWSER%, %BROWSERVERSION%, %OS%, %OSVERSION%, %USERAGENT%, %DATE%, %TIME%, %FIXTURE%, %TEST%, %TESTNUMBER%, %FILENUMBER%'
Unfortunately, we can't use
I think, we have to use shell-neutral template syntax, like Mustache. I would like to have standard JS template strings syntax: |
From
From
From
It just hangs after the QUnit line and doesn't do anything else. |
So according to @AndreyBelym's proposal the template will look like testcafe chrome test1.js -s screenshots -p "${DATE}_${TIME}/test-${TESTNUMBER}/${USERAGENT}/${TESTNUMBER}" How do you like this? |
I'm fine with that. Was also trying out using |
❌ Tests for the commit a31dc1b have failed. See details: |
I don't like this way.
I propose to use a single function (packed as module) to build a custom screenshot path. Example: testcafe chrome -s screenshots -p OrganizationShare.js
testcafe ie -s screenshots -p DevFTP.js Where /*Import necessary modules*/
export default function (runnerInfo, userAgent) {
/*Custom logic*/
return path.combine(runnerInfo.fixtureName, runnerInfo.testName, userAgent.browser...)
} Pros of this way:
What do you think about it? |
What conditional logic would you like in screenshot path template. I can't imagine a real-life case for that
It's not a problem. We normalize it by using
There is not problems with useragent since we have I see the only restriction here is it's impossible to format date. I'm not sure it's too important here. (if necessary we can add something like I feel the way with a separated But I guess we should add such function to the programming api (is the same way as we did for |
(UPDATED): Additionally we already have some kind of templating for I guess we can just extend it for a whole test run (and avoid a breaking change there). So the API can see in the following way: // CLI
testcafe chrome test.js -s <path> --screenshot-template "{currentDate}/test-{testIndex}/{userAgent}/{screenshotIndex}.png"
Available options:
- { date }
- { time }
- { testIndex }
- { screenshotIndex }
- { quarantineAttempt }
- { fixture }
- { test }
- { userAgent }
- { browser }
- { browserVersion }
- { os }
- { osVersion } // Programming
runner.screenshot(path [, takeOnFails][, filePath])
// where filePath is a template string or a function that take the info and returns a value How do you like this? /cc @VasilyStrelyaev @thireven @DevExpress/testcafe |
I don't want to invent user cases. |
My bad. We don't have templating there yet. It's just a default value example in the Take Screenshot Topic. I've updated my comment a little bit. But the proposed API is still actual |
Going off of what's there, would that mean that the pattern would change to |
Hi @thireven, After some considetaion let's stay with the following API: -p or --screenshot-path-pattern
testcafe chrome test.js -s <path> -p "${DATE}_${TIME}/test-${TEST_INDEX}/${USERAGENT}/${FILE_INDEX}.png"
Available options:
- ${DATE}
- ${TIME}
- ${TEST_INDEX}
- ${FILE_INDEX}
- ${QUARANTINE_ATTEMPT} --by default "1" (if you add it to the template but run tests without the `-q` option).
- ${FIXTURE}
- ${TEST}
- ${USERAGENT}
- ${BROWSER}
- ${BROWSER_VERSION}
- ${OS}
- ${OS_VERSION} If TestCafe creates a screenshot when a test fails (--screenshots-on-fails option enabled) we just add the |
❌ Tests for the commit 715b42f have failed. See details: |
Made some updates according to what @AlexanderMoskovkin posted and added support for custom datetime formatting (DTF). The original PR info has been updated with information on DTF and an example pattern and output. On the testing side, I'm still struggling to get two of them to succeed. |
❌ Tests for the commit 48ee194 have failed. See details: |
src/screenshots/capturer.js
Outdated
this.quarantineAttemptNum = namingOptions.quarantineAttemptNum; | ||
this.testIndex = namingOptions.testIndex; | ||
this.screenshotIndex = 1; | ||
this.errorScreenshotIndex = 1; | ||
|
||
var testDirName = `test-${this.testIndex}`; | ||
var screenshotsPath = this.enabled ? joinPath(this.baseScreenshotsPath, this.baseDirName, testDirName) : ''; | ||
var screenshotsPath = this.enabled ? this.baseScreenshotsPath : ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change leads to failures in test, see comment
Hi As you are using OSX, you do not have IE. But in the log I see the following: So, I think it should work if you remove IE (and other missing browsers) in Simply modify the testingEnvironments[testingEnvironmentNames.localBrowsers] variable value As for the gulp test-client, these tests work in a browser and it is required to open http://localhost:2000 in your browser. I have also noticed that some AppVeyour tests failed.
Please take a look at this test. it('Should take a screenshot', function () {
return runTests('./testcafe-fixtures/take-screenshot.js', 'Take a screenshot', { setScreenshotPath: true })
.then(function () {
expect(SCREENSHOT_PATH_MESSAGE_RE.test(testReport.screenshotPath)).eql(true); <---- failed here because of changes in comment
expect(assertionHelper.checkScreenshotsCreated(false, 4)).eql(true);
});
}); I have added a comment to the problematic code in the 'Files changed' tab |
❌ Tests for the commit c5eeefe have failed. See details: |
Patterns that generate the same filename will now be auto-incremented instead of being overwritten.
…arsing logic to its own file
…assuming the old format
c5eeefe
to
97d9034
Compare
❌ Tests for the commit 97d9034 have failed. See details: |
❌ Tests for the commit 77f4ddf have failed. See details: |
❌ Tests for the commit 6999614 have failed. See details: |
It will take time to resolve all the conflicts. It'll be more convenient to create a separate PR. |
I'm working on this |
Starting this PR to begin discussions about this as well as address an issue I have with the current three test suites: I'm not able to pass all the tests from a fresh clone of testcafe. I'm on OSX 13.3.3 running Node v9.2.0. All three test suites fail to complete. I can create a separate bug issue for this if necessary.
I still need to write test cases for each of these patterns which I hope to start on after resolving my issue above.
Here is the documentation I've written so far for this new feature:
Patterns
${FIXTURE}
${TEST}
${TEST_INDEX}
${FILE_INDEX}
${DATE}
${TIME}
${DTF_<FORMAT>}
${USERAGENT}
${BROWSER}
,${BROWSER_VERSION}
,${OS}
, and${OS_VERSION}
${BROWSER}
${BROWSER_VERSION}
${OS}
${OS_VERSION}
${QUARANTINE_ATTEMPT}
-q
option)Usage & Examples
Set the pattern to be used with the
-p
or--screenshot-path-pattern
flag.testcafe chrome test1.js -s screenshots -p ${DATE}_${TIME}/test-${TEST_INDEX}/${USERAGENT}/${TEST_INDEX}
(this pattern is the default one if no pattern is specified for backwards compatibility)The pattern above reproduces the default file structure for the screenshot files:
Patterns can also be used within the
takeScreenshot()
method like so:takeScreenshot('${TEST}/${OS}/${OS_VERSION}/${FILE_INDEX}')
. Note that specifying a pattern intakeScreenshot()
will override the pattern passed in the CLI for that specific screenshot.Examples of patterns and their output: