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.
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
feat(bench-test): Benchmark Testing #2720
base: master
Are you sure you want to change the base?
feat(bench-test): Benchmark Testing #2720
Changes from 9 commits
b77963a
5a5bc59
ad54242
169e0fe
80a45c3
05469f9
85727ee
c98abcd
7400948
f8f53a0
58d56d1
0586ba6
a337498
232acf6
4095f34
5ea5576
6d4f68a
e9c08da
aef3d55
da0d44f
b5a9bf0
286c377
0f9bd62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Installing
eshost-cli
does not create these variables AFAIK; they're just suggestions from its README.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.
I copied from the
eshost
with the same motivation that sometimes people change the directories manually. So, instead of a hardcode path just using a variable.But, I get the idea. I think it should be more straight forward. Changing it.
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.
I find it very strange for
yarn bench
to runnode --cpu-prof
against the hard-coded test/index.test.js file.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 is not representative of
performance.now
, which returns milliseconds.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.
Umm you are right. The name and function are conflicting.
We just want to measure the time in nano-seconds. So, if anything's gonna change it's the name of the function.
Please, do post any suggestions related to precise time measurement.
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.
Changed the name.
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.
@michaelfig @gibson042 , do you agree that the lint complaint about the
await
on this line 7 is incorrect and a bug in our lint rule, since the earlier top-levelawait null;
on line 4 should have closed the synchronous prelude?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.
Huh. Or maybe the "no await inside a loop" is a pre-existing lint setting separate from any of our
await
safety rules? In which case we should turn off this pre-existing lint rule globally, i.e., everywhere our ownawait
safety rules are enforced? Attn @kriskowal @mhofman @turadgThere 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.
https://eslint.org/docs/latest/rules/no-await-in-loop is built into Eslint. It's not for safety so I don't think the Jessie rule affects whether to enable it.
btw, I don't see any linting output in CI because of this error:
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.
I am of this opinion that we should disable
no-await-in-loop
rule for this line. Reason being, we actually need to check the performance of the function execution separately.@turadg the issue you mentioned in the code snippet above is being resolved but the workflow has not been ran after the update.
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.
@turadg writes:
Correct. I consider it a reminder for folks to use
for-await-of
instead if that makes the code more straightforward.@muhammadahmadasifbhatti writes:
I agree with that opinion.