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
test: integrate runtime-tests as a helper library #549
test: integrate runtime-tests as a helper library #549
Changes from 25 commits
c19211c
1c1cca7
83cba6f
ff4dd7b
169120f
31761ba
9ad3233
11a0c8e
dbe4d1d
18d8050
6631813
bd0333e
8731c57
77266a7
58ff145
6a4b149
6f2c253
19b6f3e
5c2023f
67896bb
a876e73
d4fccec
73ea3dd
ab7833e
e098fb2
a6ccdad
29d7f82
414b7ae
ec9a028
de4184b
491cee5
c9a914b
7f73b6c
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.
Not sure I understand what this does. What is the expected input here? It walks all cli arguments, filters out anything that starts with "/", sets the
config[cmd]
as it goes? How is it used?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.
Yea this can be a bit confusing.
Parses out all inputs that start with
/
. (This is so we dont save things such as the path of the node.js binary into the config)For everything else If the input doesn't start with
--
, then we return""
(This sets the accumulator in this casecmd
to false). This will resolve to a falsey value which will stop the reducer from saving it into the config.argv.push(arg)
is used to save any local config options in order to pass it into the jest binary such as--config=./runtime-tests/jest.config.js
.If the
arg
is equal to--
and has the name ofchain
then it will set it equal to the accumulator orcmd
and then the arg that follows will be the argument for the command itself.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.
Hmm, I wonder if we're trying to be a bit too clever for our own good here; feels like this should be easier to do? ("no it's just you being thick" is a valid answer here!).
I was actually wondering something more basic: what is this args processing for? Is this right (or close to right)?
"Process the command line arguments used to launch the test session. Merge any config options provided at the command line with the on disk config."
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.
Yea so you actually make a great point and I was able to incredibly simplify it. This method is way too clever for what it is suppose to be. I basically just wrote out a custom parser which doesn't need to be done either.
I chose to do it this way because there is no straight forward way to pass custom CLI commands to jest other than setting an entry file, and calling the binary after adding a custom configuration to the disk. But that being said I also could have just used a arg-parser and then directly pass it in same as I did. Which is exactly what I changed it too. I added
argparse
now and have it just be the same as any other node argument parsing tool. No loops, or complicated logic anymore.Good catch. Thanks
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 used to seeing
reduce()
return the accumulator at every iteration (including the last) and assign the return value to a variable, e.g.let sum = [1,2,3].reduce((acc, x) => x*x)
. Instead here it seems like you're not using the return value of the wholereduce
and relying instead on side-effects like this one. Should it be a for-loop then?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.
So in this case I am using the reducer a little differently than normal, the accumulator in this case is being used to identify falsey values, vs truthy commands. I stayed away from the
for loop
because I didnt want to introduce messy iterator manipulation in order to parse the args, and I think the reducer itself allows for more customization for the future in case we wanted to add arguments to the script.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.
Would be cool if we had our own nodes - but I supposed you could not check the addresses into source control.
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 could input the URLs as either
CLI args
or setenv variables
for them instead.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.
Yea I agree having our own nodes would be very nice. Would be a nice addition for the tools team in general.