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 19 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.
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'm not comfortable with
yarn test
modifying $HOME.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 get the idea but what we are gonna do for the CI? As per your suggestion, we need to update the
ci.yaml
to installesvu
and then install the engines usingesvu
. But, for this particular PR, I am not comfortable with changing theci.yaml
file. Open for the suggestions.Nevertheless, it's gonna change in the next couple of PRs when we will be using
node
andxsnap
to run these tests.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.
Changing ci.yaml is entirely appropriate; this adds a new requirement on the host environment just like the one currently fulfilled by setup-node.
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.
And for local purposes, a package.json script for use like
yarn run install-engines
would make the consequences more clear.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 still having second thoughts about this change. Reason being, there are follow-up tasks aligned and this code might not even exist in the next PR. Also, if we make this change we have to reverse the
ci.yaml
?Another reason is
ci.yaml
is being run for every package in thepackage/
directory. So, it will be an unnecessary side effects for other packages to installesvu
andeshost
globally.@turadg had a similar idea in his mind to not install
esvu
andeshost
globally. Link to the comment.I am more inclined towards having different script (as you mentioned in the second comment).
yarn install-engines
which will do all theesvu
andeshost
related stuff.yarn test:bench
to run the benchmark tests. In this case bundle the code and run the tests inindex.test.js
yarn test
to runyarn install-engines
andyarn test:bench
. Might be vague in names but open for suggestions.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.
But, even with different scripts
yarn
will be modifying the$HOME
directory.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.
Yes. You can add a comment as a reminder.
That's not what I'm seeing at https://github.com/endojs/endo/blob/master/.github/workflows/ci.yml. Only the job "test" runs
yarn test
at package scope, and it does so after some setup steps. So what I'm asking for is insertion of esvu engine installation and eshost setup before itsyarn build
step, which will run exactly as often as the rest of those steps (i.e., once for each item of the (node-version: [18.x, 20.x, 22.4.1]
,platform: [ubuntu-latest]
)matrix
cross product).Having the separate package.json script is good, and the CI job step could even use it to reduce duplication. But the step on its own does not resolve this issue, because CI must have the engines but local
yarn test
must not install them.No, I'm still objecting to
yarn test
modifying $HOME.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'm not comfortable with
yarn test
modifying $HOME.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.
Addressed above.
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.
What purpose is served by these commands?
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.
The first three are the logs for the debugging purposes. I still wanted to have them in case something goes off after merging the PR in the CI.
In the last two lines, we are changing the permissions of engine binaries so that we can execute them to run our bundled.
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.
But they won't serve that purpose without either a preceding echo or a
set -x
.Aren't they already marked executable when installed? I've never had to chmod them locally.
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 able to see the logs both on local and on ci. Nevertheless, pushing an

echo
because that seems right.Nope, I had to change the
mod
locally. Also, at first I pushed the changes without changing themod
and got an error on theCI
i.e. Either the file does not exist or it is not executable.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.
Again, I did this for the CI.
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.
But it shouldn't degrade the local experience. Something like this would still accomplish CI visibility:
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 think we are not on the same page here.
Let's suppose I accept the suggested changes. Now, the
CI
will runyarn test
and it we will immediately faill because of errorxs and/or v8 not known to eshost; please eshost --add them per README.md.
This will work only if we update the
ci.yaml
to run these commands (of adding the engines) for us.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.
Yes, that's exactly what I'm requesting.
yarn test
on its own requires the engines to already exist and fails with an informative message when they don't. ci.yaml makes sure that the engines are installed before invokingyarn test
.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.