Init script for the reassure cli package #275
Conversation
|
|
@mdjastrzebski @thymikee WDYT? |
Performance Comparison ReportSignificant Changes To Render Duration
Show details
Meaningless Changes To Render DurationShow entries
Show details
Changes To Render CountThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
Xiltyn
changed the title
|
@Xiltyn You can do the docs separately. |
|
@Xiltyn re ESM: I recall that going full ESM for libraries is a bit more complicated, with some drawbscks for users. We could migrate only Reassure-CLI to ESM, but that is low priority, there are more interesting tasks to do if you have some time to spare :-) |
|
@mdjastrzebski Absolutely, it's not like I'm super into the migration necessarily, but it was suggested by @thymikee to me so I wanted to float that idea for the future. Wasn't aware of user-related drawbacks :) thx! |
|
Migration to ESM is something to keep in mind and we should put it on our backlog somewhere. I'm a fan of scoping the tightly-coupled work together, so I'd personally include the docs along the feature. This is not a requirement however, and if it speeds up your work by splitting it up, please do :) |
|
Btw, this needs a rebase |
|
Huh, that's true. I'll do the rebase. As for the docs, we'll do them separately as part of my next OSS day. Mainly because it's hard for me to fully commit to a given date atm and I don't wanna keep it on hold. Rebase should be quick, I'll do my best to have it done today eod. |
Xiltyn
force-pushed
the
feature/cli-init-script
branch
from
3afcc69
to
1504d57
Compare
|
Rebased. Please review if you have a couple minutes so that we can finally merge it. |
dangerfile.reassure.js
Outdated
| "use strict"; | ||
|
|
||
| var _path = _interopRequireDefault(require("path")); | ||
| var _reassure = require("reassure"); | ||
| function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; } | ||
| /* eslint-disable import/no-extraneous-dependencies */ | ||
|
|
||
| (0, _reassure.dangerReassure)({ | ||
| inputFilePath: _path.default.join(__dirname, './examples/native/.reassure/output.md') | ||
| }); | ||
| //# sourceMappingURL=dangerfile.js.map |
There was a problem hiding this comment.
I think this file shouldn't end up here
There was a problem hiding this comment.
Thanks for spotting, you are correct. Pushed a fix removing this one.
Docs change added to this one since it's still open :) |
|
I added an optional parameter to the init command I also set the templates to have no extension at all, which allows them to be ignored by babel and to not be compiled. Ultimately, I think, it would be good to set a sort of a fake extension on the templates. For example Wdyt? |
Xiltyn
force-pushed
the
feature/cli-init-script
branch
from
7273261
to
1f9e451
Compare
|
Rebased with main |
docusaurus/docs/installation.md
Outdated
| ``` | ||
| ├── <ROOT> | ||
| │ ├── .reassure | ||
| │ │ └── reassure-tests.sh |
There was a problem hiding this comment.
Why is reassure-tests.sh put under .reassure? reassure-tests.sh should be. top level.
| ├── <ROOT> | ||
| │ ├── .reassure | ||
| │ │ └── reassure-tests.sh | ||
| │ ├── dangerfile.ts (conditional) |
There was a problem hiding this comment.
| │ ├── dangerfile.ts (conditional) | |
| │ ├── dangerfile.ts (append) |
There was a problem hiding this comment.
It's not an append, we cannot append automatically, this would have to be a separate feature as it would require us to separate bits of our dangerfile template into two pieces I think. Then we could properly append imports on top and our function on the bottom.
Unless it doesn't matter to us, but then we'll be polluting ppl's dangerfiles which I would deem a bad UX/DX
wdyt? @mdjastrzebski @thymikee
There was a problem hiding this comment.
Yeah, the current solution make sense.
| │ ├── dangerfile.ts (conditional) | |
| │ ├── dangerfile.ts (or dangerfile.reassure.ts if dangerfile already present) |
| @@ -146,7 +172,7 @@ yarn install --force | |||
| yarn reassure | |||
| ``` | |||
|
|
|||
| ## CI integration | |||
| ### Integration | |||
|
|
|||
| As a final setup step you need to configure your CI to run the performance testing script and output the result. | |||
| For presenting output at the moment we integrate with Danger JS, which supports all major CI tools. | |||
There was a problem hiding this comment.
Can we redact that, so that it mentions dangerfile.reassure.ts?
There was a problem hiding this comment.
I can reiterate so that it's there as well, but i's been mentioned already above in the same encompassing section.
There was a problem hiding this comment.
I re-edited that section in order to include clearer instructions about it
| }; | ||
|
|
||
| console.log(`\n❇️ Running performance tests:`); | ||
| console.log(` - ${measurementType}: ${formatMetadata(metadata)}\n`); | ||
| warn(`❇️ Running performance tests:`); |
There was a problem hiding this comment.
Why warn? It seems like this should be regular log.
There was a problem hiding this comment.
Same as the next comment but in this case I think that you are in fact correct. I will ammend
| return; | ||
| } | ||
|
|
||
| if (options.baseline) { | ||
| console.log("Hint: You can now run 'reassure' to measure & compare performance against modified code.\n"); | ||
| warn("Hint: You can now run 'reassure' to measure & compare performance against modified code.\n"); |
There was a problem hiding this comment.
Visibility and styling. log will be shown only in --verbose and warn shows always unless you run with --silent. I thought that;s a more important log than most general procedural logs.
…g the init command allowing them to not be transpiled by babel
…tween TS and JS for the dangerfile template
Co-authored-by: Maciej Jastrzebski <mdjastrzebski@gmail.com>
Co-authored-by: Maciej Jastrzebski <mdjastrzebski@gmail.com>
Xiltyn
force-pushed
the
feature/cli-init-script
branch
from
1a33eb6
to
aedaaef
Compare
| await mkdirSync(RESULTS_DIRECTORY); | ||
|
|
||
| logger.log('Copying the reassure-tests.sh template file'); | ||
| await copyFileSync(`${__dirname}/../templates/reassure-tests`, `./reassure-tests.sh`); |
There was a problem hiding this comment.
doesn't need await right? same as other *Sync methods, can you remove all unnecessary occurrences?
There was a problem hiding this comment.
I think I took the the other way around. Thanks for pointing it out!
There was a problem hiding this comment.
I definitely did take it the wrong way around. It was fixed in this commit
|
I'll try to review it in the upcoming days. |
|
|
||
| #### `.gitignore` | ||
|
|
||
| If .gitignore file is present and no mentions of `reassure` appear within it, the script will append the `.reassure/` directory to its end. |
There was a problem hiding this comment.
| If .gitignore file is present and no mentions of `reassure` appear within it, the script will append the `.reassure/` directory to its end. | |
| If `.gitignore` file is present and no mentions of `reassure` appear within it, the script will append the `.reassure/` directory to its end. |
|
|
||
| If .gitignore file is present and no mentions of `reassure` appear within it, the script will append the `.reassure/` directory to its end. | ||
|
|
||
| #### `reassure-tests.sh` |
There was a problem hiding this comment.
Looks like this file will be generated by Reassure init. We could keep this section brief, mention the purpupose of that file and that it will be generated by Reassure init, while having more details in the github docs. @Xiltyn wdyt?
|
That's fair, I kinda defsulted to having a mirror of relevant sections
here and there, but I don't mind keeping it more concise here.
…On Mon, 20 Mar 2023, 18:29 Maciej Jastrzebski, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In README.md
<#275 (comment)>:
> +
+#### `--javascript` (optional)
+
+By default Reassure scripts will generate TypeScript files. You can use this option, if you'd like to generate JavaScript files instead.
+
+### Scaffolding
+
+#### `dangerfile.ts and dangerfile.reassure.ts`
+
+If your project already contains a `dangerfile`, the CLI will not override it in any way. Instead, it will generate a `dangerfile.reassure.ts` file which will allow you to compare and update your own at your own convenience.
+
+#### `.gitignore`
+
+If .gitignore file is present and no mentions of `reassure` appear within it, the script will append the `.reassure/` directory to its end.
+
+#### `reassure-tests.sh`
Looks like this file will be generated by Reassure init. We could keep
this section brief, mention the purpupose of that file and that it will be
generated by Reassure init, while having more details in the github docs.
@Xiltyn <https://github.com/Xiltyn> wdyt?
—
Reply to this email directly, view it on GitHub
<#275 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AELZTCI3JYQ3VXVFGARDQG3W5CHW7ANCNFSM6AAAAAASOY5VNA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
The PR adds many valuable features (e.g. init), however I have concerns about some proposed solutions:
- Redefining the default behavior of
reassurethat now runs interactive prompter. Imo we should keep the most used option of running test in the spirt ofjestbeing the command to run tests. Changes here will break the user scripts (incl. the default ones that useyarn reassure. - Interactive prompter seems fine for
initscript, but feels weird for other commands. Imo I would leave it only forinitOR if you find it useful then make it optional for other commands, e.g. behind--interactiveoption. - ASCII art looks nice for
initcommand, but pollutes the log formeasureand other commands. I would suggest displaying it only forinitOR for--interactivemode if we go that way. That should also lead to removing of--no-ascii-artoption as no longer needed. --subroutineoptions is hard to understand even with CLI description text. Is this really needed?- Logger uses
reassure:prefix, which we agreed with @thymikee in feat: logger #343 that necessarily pollutes the logs.
@Xiltyn @thymikee Pls weight in on the above points and also if we should allow interactive mode for commands other that init.
| ], | ||
| } | ||
| ``` | ||
| > **Note**: Your performance test will run much longer than regular integration tests. It's because we run each test scenario multiple times (by default 10), and we repeat that for two branches of your code. Hence, each test will run 20 times by default. That's unless you increase that number even higher. |
There was a problem hiding this comment.
| > **Note**: Your performance test will run much longer than regular integration tests. It's because we run each test scenario multiple times (by default 10), and we repeat that for two branches of your code. Hence, each test will run 20 times by default. That's unless you increase that number even higher. | |
| > **Note**: Your performance test will run significantly longer than regular integration tests. It's because we run each test scenario multiple times (by default 10), and we repeat that for two branches of your code. Hence, each test will run 20 times by default. That's unless you increase that number even higher. |
| ├── <ROOT> | ||
| │ ├── .reassure | ||
| │ │ └── reassure-tests.sh | ||
| │ ├── dangerfile.ts (conditional) |
There was a problem hiding this comment.
Yeah, the current solution make sense.
| │ ├── dangerfile.ts (conditional) | |
| │ ├── dangerfile.ts (or dangerfile.reassure.ts if dangerfile already present) |
| You can also check our example [Dangerfile](https://github.com/callstack/reassure/blob/main/dangerfile.ts). | ||
| #### Creating a new `dangerfile` | ||
|
|
||
| If you do not have a dangerfile yet, you can use the one generated by the `reassure init` script without making any additional changes. |
There was a problem hiding this comment.
| If you do not have a dangerfile yet, you can use the one generated by the `reassure init` script without making any additional changes. | |
| If you do not have a `dangerfile` yet, you can use the one generated by the `reassure init` script without making any additional changes. |
| await measure({ ...options, baseline: false, compare: true }); | ||
| hello(options); | ||
|
|
||
| await measure({ baseline: true, isSubRoutine: true }); |
There was a problem hiding this comment.
- Why are options no-longer passed to
measure? isSubRoutinename is not hard to understand without context.
| }) | ||
| .option('testMatch', { | ||
| }, | ||
| testMatch: { |
There was a problem hiding this comment.
Seems we are getting into two conflicting option naming styles: --testMatch vs --commit-hash. @Xiltyn any thoughts on that.
| @@ -23,6 +22,7 @@ interface MeasureOptions extends CommonOptions { | |||
|
|
|||
| export async function run(options: MeasureOptions) { | |||
| configureLoggerOptions(options); | |||
| hello(options); | |||
There was a problem hiding this comment.
IMO we should not runt hello or bye by default for any other commend beside init as they will be annoying to the user.
| bye(); | ||
| } | ||
|
|
||
| export const command: CommandModule<{}, {}> = { |
There was a problem hiding this comment.
I am against having the interactive mode by default. It will break the simplicity of running yarn reassure for testing. I would reserve prompting for the init command as this the command that users will primarily do by hand. Otherwise, we could leave it as optional --interactive mode, but IMO the default stay the same.
| const colorVerbose = chalk.hex(colors.verbose); | ||
| const colorVerbose = chalk.hex(colors.default); | ||
|
|
||
| const prefix: string = colorBrand('reassure: '); |
There was a problem hiding this comment.
We agreed with @thymikee that prefix is not really useful and pollutes the logs.
| /** No logs */ | ||
| silent?: boolean; | ||
| /** No Ascii art */ | ||
| 'no-ascii-art'?: boolean; |
There was a problem hiding this comment.
IMO I would drop this option, and display ascii art only either only for CLI init OR only for interactive prompts if we go that option.
|
Based on today's conversation with @Xiltyn, we agreed to split this PR into smaller, orthogonal ones that handle separate PRs ( |
|
Closing in favour of #381 which contains just the |
Init script for the reassure cli package
Summary
More information and acceptance criteria found @ #222
Test plan
You can run the CLI init script using
yarn reassure initcommand. Use the availablelogLevelcli argument to see different levels of logsverbose,defaultorsilent.Happenstance findings
chalkis locked at its max version 4. We cannot bump to 5 as we do not support esModules.@mdjastrzebski I suggest creating an issue to actually migrate the codebase to esModules as this seems to be the way to go nowadays? Wdyt?
Demo video
reassure_init.mov
Updated styling for the ASCII art after the video was recorded