feat: add support for directory and handle piped files #80
Conversation
src/cli.ts
Outdated
| } | ||
| }); | ||
| requireSuites(suites); | ||
| } else if (program.inline) { |
There was a problem hiding this comment.
@andrewvc I introduced a new flag inline as we want a distinction between the content that is coming as source code (string) vs the actual source which is a proper JS. stdin felt different as we can pipe any content which is handled differently when done like this
> ls /examples/*.js | npx @elastic/synthetics
There was a problem hiding this comment.
I'm thinking we should change the naming. The --inline flag comes from heartbeat where the source was "inline" with the YAML. I think it's confusing outside of that context. I think a better term is single, since it's a single journey.
There was a problem hiding this comment.
We need distinction between single journey files vs JS string code coming from the heartbeat. The --inline flag here only deals with the later. We have first class support for files without passing any flags. Are you proposing to change this flag from --inline to --single ?
There was a problem hiding this comment.
Yes, just a rename. Thoughts on naming? CC @jahtalab @paulb-elastic @justinkambic
There was a problem hiding this comment.
I prefer inline over single, as single can both denote single file or single journey from heartbeat. But not opposed with single either.
💔 Build Failed
Expand to view the summary
Build stats
Test stats 🧪
Steps errors
Expand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
|
Test failures are unrelated to the PR. |
|
@andrewvc The PR is ready for review |
There was a problem hiding this comment.
Generally looking good, raised some questions on some points. Also, can convert the elastic-docs and sample apps to use this format?
I think we'll learn more about this pattern, particularly about hooking in to start the ruby web server. I'm thinking it may make sense, given these changes to add {before|after|around}{Suite|Step} hooks to accomplish these sorts of things, similar to how jest does it. We can look at jest and similar as inspiration.
Also, can we include one typescript example, with a shell script included showing how to run those with the special CLI flag?
src/cli.ts
Outdated
| } | ||
| }); | ||
| requireSuites(suites); | ||
| } else if (program.inline) { |
There was a problem hiding this comment.
I'm thinking we should change the naming. The --inline flag comes from heartbeat where the source was "inline" with the YAML. I think it's confusing outside of that context. I think a better term is single, since it's a single journey.
Will do once we add support for this #84. Makes it easier
Totally agree, But we can get around that for now by moving them before steps as a interim solution and add the hooks later on. WDYT?
Guess examples have to wait until the support for |
There was a problem hiding this comment.
@vigneshshanmugam , As we discussed on the tech sync, we should add support for path wildcards such as the *.journey.js (it can be more complex than this) to the command line arguments (and possibly remove the --pattern option). This would be simpler compared to using command line pipes.
I'm happy to create an issue for this enhancement if you think it doesn't fit into this PR.
|
@jahtalab I thought about it and tested, glob patterns parsing and matching is harder and more slower than pattern matching (We can use library). We have to pay super huge time at startup cost which i personally don't think superior considering most patterns can be done using simple pattern matching. Please create an issue, we can discuss and get others opinion as well before introducing it. |
|
Thanks @vigneshshanmugam for testing it. Do you mind posting the benchmark results here for reference? this would make it easier to discuss later. |
|
@jahtalab Sure, I will post the numbers on the new issue with startup cost for regex and glob patterns. Have diverged a bit currently and it would need evaluation for the filtering tag/names on journey logic as well if we need to support glob patterns. |
|
I'm OK with us using regexps instead of globs. In fact, jest does this exact thing. In fact, maybe we should do that in #26 as well. While globs are more standard in UNIX tooling, they are somewhat awkward in JS. I doubt the perf difference is meaningful here, but skipping the dependency is probably worth it. I'm OK keeping |
|
Just to clarify, if we go with the regexp I think that resolves the outstanding questions enough for me, but we need to really think about the suite use case. If we're supporting directories of files let's move our examples over to that and add supporting features like |
|
@andrewvc Agreed with your points on moving the examples and supporting hooks. I have already started porting our examples in the PR #87 and didn't do in this one specifically it felt the change is already introducing too many flags, also provided an example of how a JS only version would work and did not want to do a massive PR. That said, will address all of the comments in that PR which has the required Merging this PR as it is and would move the examples in the follow up PR #87 |
--patternflag to allow filtering based on specific files inside the directory. Only applicable when directory is passed.--inlineflag as we want a way to distinguish proper node code vs source code being passed as string.Different ways you can run synthetics now