feat(screenshot): Add screenshot function to e2e test (button and checkbox) #2532
Conversation
googlebot
added
the
cla: yes
| get filename() { | ||
| return this.id | ||
| .toLowerCase() | ||
| .replace(/[ :\/]/g, '_') |
There was a problem hiding this comment.
Can change the space to \s to capture all whitespace.
| * The filename used to store the screenshot | ||
| * @returns {string} | ||
| */ | ||
| get filename() { |
There was a problem hiding this comment.
Can do get filename(): string and remove the type from the JsDoc
(here and elsewhere for return types and params)
| * The full path to the screenshot | ||
| * @returns {string} | ||
| */ | ||
| get path() { |
|
|
||
| describe('button', function () { | ||
| describe('disabling behavior', function () { | ||
| beforeEach(function() { | ||
| browser.get('/button'); | ||
| }); | ||
| it('should prevent click handlers from executing when disabled', function () { | ||
| let testname = |
There was a problem hiding this comment.
Jasmine 2.0 made it more difficult to get the current test name, but it's still possible by adding a custom reporter. Example.
Could could add a reporter that just updates a value which is then read in screenshot.ts
There was a problem hiding this comment.
Added a reporter and we can use the test name in Screenshot now
| if (!fs.existsSync(dir)) { | ||
| fs.mkdirSync(dir, 0o744); | ||
| } | ||
| fs.writeFileSync(this.path, png, {encoding: 'base64'}); |
There was a problem hiding this comment.
Why base64 encoding? Shouldn't this be binary?
There was a problem hiding this comment.
The output of binary cannot be opened as an image.
| storeScreenshot(png: any) { | ||
| var dir = path.resolve(__dirname, '..', 'screenshots'); | ||
| if (!fs.existsSync(dir)) { | ||
| fs.mkdirSync(dir, 0o744); |
There was a problem hiding this comment.
Why octal instead of hexadecimal?
tinayuangao
force-pushed
the
screenshot-pr
branch
6 times, most recently
from
eb60e39
to
2b4e66d
Compare
tinayuangao
force-pushed
the
screenshot-pr
branch
from
6b45e60
to
a48c356
Compare
tinayuangao
changed the title
|
Fixed test on travis-ci and sauce labs. Please take another look. Thanks! |
|
|
||
| export class Screenshot { | ||
| id: string; | ||
| dir: string = '/tmp/angular-material2-build/screenshots/'; |
There was a problem hiding this comment.
Make this a file-level constant, e.g.,
/** Directory into which screenshots will be written. */
const OUTPUT_DIR = '/tmp/angular-material2-build/screenshots/';| function initializeEnvironment(jasmine: any) { | ||
| var reporter = new jasmine.JsApiReporter({}); | ||
| reporter.specStarted = function(result: any) { | ||
| jasmine.getEnv().currentSpec = result.fullName; |
There was a problem hiding this comment.
Instead of storing this to jasmine.getEnv(), would it work to create a variable at the file level and update that? E.g.,
let currentJasmineSpecName = '';
/** Adds a custom jasmine reporter that simply keeps track of the current test name. */
function installJasmineReporter(jasmine: any) {
jasmine.getEnv().addReporter(new jasmine.JsApiReporter({
specStarted: (result: any) => {
currentJasmineSpecName = result.fullName;
}
}));
}There was a problem hiding this comment.
I also just realized: we should make sure that the test results are still being reported normally with the custom report installed
There was a problem hiding this comment.
Changed to file-level variable.
Constructor JsApiReporter({specStarted}) is not working.
Tested the test results are being reported normally.
tinayuangao
force-pushed
the
screenshot-pr
branch
2 times, most recently
from
53135ce
to
1670ee4
Compare
tinayuangao
force-pushed
the
screenshot-pr
branch
from
c312aa6
to
268c55a
Compare
jelbourn
added
pr: lgtm
action: merge
|
@jelbourn @tinayuangao I just have some questions:
|
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
R: @jelbourn