closes #466 add log file v4 #517
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I'm thinking this is just going to be for v4. Not bothering with this for v3 unless we absolutely have to since none of the vulnerabilities are severe. My current rationale is that the more work we do to maintain 3, the less work we can do getting v4 ready for release. Ready to hear opinions. - Close #164, update cucumber to v7 - Prepare for v8 of cucumber because I won't remember it later - Close #394, update puppeteer - Update our version of node (and that of our action that we'll run for other people's libs). [Addresses #393 so we can use the suffolk npm org package.] - Use `npm audit` to fix the remaining vulnerabilities (now 0) - [Remove package.json as discussed in #489 to align our tests' behaviors with those of our users.] * Update action.yml node to v17 * Update from cucumber v6 to v7. See details. See https://github.com/cucumber/cucumber-js/blob/main/docs/migration.md#migrating-to-cucumber-js-7xx Only use cucumber setDefaultTimeout globally and use a shim that replicates the fix in v8 that lets you do custom timeouts more easily so we can still give enough time for steps that may need more time. Use all caps for statuses. Test screenshot step. Btw, the cucumber test output visually looks a bit different now - when a scenario passes, all the steps pass too. Sorry about the little comment changes, etc. Tried to remove a lot of those incidental unrelated changes. * Update puppeteer to latest (13). See details below. - page.waitFor -> page.waitForTimeout and page.waitForSelector (Got deprication notice. See puppeteer/puppeteer#6214.) - remove removeEventListener (we'd need to change it to removeListener anyway - v4.0.0 and see https://github.com/puppeteer/puppeteer/blob/main/docs/api.md#eventemitterremovelistenerevent-handler). For now we'll count on page close taking care of it, just in case removing it would prevent multiple-file-downloads. * Update GitHub worflow node version, tweak changelog item order * Fix npm audit vulnerabilities and update action.yml cucumber * Pin the colors lib in action.yml * Remove package-lock.json #489, use kiln v4 for users, CHANGELOG * Fix custom timeout, remove duplicate report entry, as per review
Closes #387. Also, generally adds safety measures for when page does not exist. Very similar to PR #459, but moving the responsibility down to a spot that most functions make use of, meaning that it'll be applied to a lot more cases. They mostly won't need it, but it might still be worth being more comprehensive. * Allow a developer to wait as a first Step v4. #387. Add test. Will be able to close once we've added this as an establishing step (in addition to it being a regular step). Also, generally adds safety measures for when page does not exist. * Add test Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com> Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com>
plocket
requested changes
Feb 18, 2022
There was a problem hiding this comment.
Great progress! A couple things to add/change that I completely forgot to describe to you. Some of them are nitpicky and if you don't get to them, I'll approve the PR anyway.
When the major items are taken care of, I'll update the changelog because it has a merge conflict from other branches that were recently merged.
log.txt
Outdated
| @@ -0,0 +1,909 @@ | |||
| Report: | |||
There was a problem hiding this comment.
I totally forgot to mention this, but we need to make sure this file doesn't get checked in (for our own sanity). Go ahead and delete this file and add ./log.txt to the bottom of the .gitignore file.
lib/steps.js
Outdated
| @@ -57,6 +57,8 @@ BeforeAll(async () => { | |||
| scope.timeout = 30 * 1000; | |||
| scope.showif_timeout = 600; | |||
| scope.report = new Map(); | |||
| fs.writeFileSync('log.txt', " "); | |||
There was a problem hiding this comment.
Is it necessary to put a character into this file or can it be an empty string? If so, that's ok. If not, it's a bit nit-picky, but I think it'd look better to remove it.
There was a problem hiding this comment.
Yes no problem
lib/steps.js
Outdated
| @@ -57,6 +57,8 @@ BeforeAll(async () => { | |||
| scope.timeout = 30 * 1000; | |||
| scope.showif_timeout = 600; | |||
| scope.report = new Map(); | |||
| fs.writeFileSync('log.txt', " "); | |||
| console.log("file written successfully"); | |||
There was a problem hiding this comment.
Looks like it was good info for development, but we can now delete the console.log in here and in scope.js so users don't see the messages.
lib/scope.js
Outdated
| @@ -32,7 +32,10 @@ module.exports = { | |||
| let scenario = await scope.makeSureReportPartsExist( scope ); | |||
| scenario.lines.push({ type, value }); | |||
|
|
|||
| fs.appendFileSync('log.txt', `Report:\n type: ${ type }, value: ${ value }`); | |||
There was a problem hiding this comment.
This is my typo from my console log, but it'd be generous of you to fix it - I have a space between the \n and "type". This is what I'd love it to be:
Suggested change
| fs.appendFileSync('log.txt', `Report:\n type: ${ type }, value: ${ value }`); | |
| fs.appendFileSync('log.txt', `Report:\ntype: ${ type }, value: ${ value }`); |
plocket
approved these changes
Mar 2, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Created log.txt and git hub artifact for reports. Closes #466.