feat: implement status report #224
Conversation
puskuruk
force-pushed
the
implement-status-report
branch
from
1bbbede to
f2891db
Compare
puskuruk
changed the title
There was a problem hiding this comment.
Thanks for the contribution! This is exactly how I imagined the status report would work :)
I've added a few comments and nits. Though, in overall, I'd like @OrKoN to review this CL as well especially for the usage of 3rd party libraries.
In addition to that, I have a few more suggestions somewhat related to this feature but not in the scope of this PR:
- Maybe we can combine the status report with "run" status. For example, when the user has run the CLI for some recordings, instead of logging "Running the recording X" and "Finished run X" we can directly present the table like
| Recording Title | Status | Path etc |
|---|---|---|
| My Recording | Running | ... |
| 2nd recording | Pending | ... |
- At the end of the run, until the control returned back to the user (while the browser is being closed) there is a time. It feels like the run has stuck at that moment, maybe we can provide some output for saying something like "Cleaning up the run..."
| @@ -112,9 +180,19 @@ export async function runFiles( | |||
| opts.log && console.log(`Finished running ${file}`); | |||
| } catch (err) { | |||
| opts.log && console.error(`Error running ${file}`, err); | |||
| throw err; | |||
There was a problem hiding this comment.
This change changes the behavior of the CLI:
- Before: when a replay is failed, we were throwing error from here so that the
yargslibrary was returning the status code1to the bash - With this change: even though a replay is failed, we now return control to the user with status code
0.
We want to return status code 1 when a replay is failed since this makes the library more useful especially in CI workflows (i.e. if a replay is failed, CI will report the task to be failed as well)
So, can you restore the old behavior of throwing an error? (It's fine running all the results, I guess at the end of all runs, we can check whether there is a replay result with failure and if so throw an error -- though up to you 😂)
|
Thank you for your review @ergunsh 🙏 I liked your suggestion of showing the current status of each recording dynamically. I didn't implement it before but I've started to look at what are the approaches for implementing dynamic logs. |
puskuruk
force-pushed
the
implement-status-report
branch
from
1578c5d to
cccca54
Compare
OrKoN
force-pushed
the
implement-status-report
branch
from
cccca54 to
bc891d9
Compare
OrKoN
force-pushed
the
implement-status-report
branch
from
bc891d9 to
a2b51a1
Compare
|
Thanks for working on this! |
puskuruk
force-pushed
the
implement-status-report
branch
from
a2b51a1 to
3add140
Compare
|
The branch was behind the |
|
@puskuruk it looks like the color.js is not the best module from a security perspective https://security.snyk.io/vuln/SNYK-JS-COLORS-2331906 Could we replace it with https://github.com/jorgebucaran/colorette or another alternative? |
|
Nice catch @OrKoN! I'll update it ASAP 🙏 |
This comment was marked as spam.
This comment was marked as spam.
Implements #133