Skip to content
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

Benchmark rw test ... duration for case of Test-project #4504

Merged
merged 6 commits into from
Feb 23, 2022

Conversation

thedavidprice
Copy link
Contributor

closes #4356

This adds timedTelemetry to yarn rw test only when --no-watch is used.

I will use this with the data from the new "CLI Checks and Telemetry Benchmarks" GitHub Workflow to set a benchmark we can watch for each case:

  • yarn rw test web
  • yarn rw test api

@thedavidprice thedavidprice added the release:chore This PR is a chore (means nothing for users) label Feb 16, 2022
@thedavidprice
Copy link
Contributor Author

@cannikin I've tested this locally to confirm we are receiving duration data for type=test 🚀

Couple of hiccups I could use your eyes on.

1. error with info not found
During the test run, it seems that info.npmPackages['@redwoodjs/core']? is err'ing. Doesn't block the process. Just strange that it's happening and curious if you have any ideas as to what's going on.

Error in CI here:
https://github.com/redwoodjs/redwood/runs/5223142960?check_suite_focus=true#step:12:78

2. type errors in sendTelemtry
Quick glance through the code and couldn't tell if these lines are correct. Could you verify these the three lines in this file, each marked with a // TODO ...?
https://github.com/redwoodjs/redwood/pull/4504/files#diff-3a3ffc02e7928c5bef19faba4c5c84dab5aba6a86fe1a4d8c12fbada4d3d99f6

@cannikin
Copy link
Member

  1. So it's complaining that info.npmPackages is undefined, hmmm...is this something about how Just executes, it runs in a separate fork or something, so that when that process forks sendTelemetry it's not actually running in the context of a regular node process anymore, so info.npmPackages is just non-existent?

  2. The first one about redwoodVersion not existing, that's where the previous error is being thrown right? For the rest, if the command that's running (test in this case) isn't in the list of SENSITIVE_ARG_POSITIONS then sensativeCommand should be undefined. Maybe line 98/99 just needs an | undefined added? But I'm not the type guy!

@thedavidprice
Copy link
Contributor Author

  1. Yeah, exactly. I don't think there's anything we can do about it in this case. Surprised it's happening but was assuming something like you describe above. It's not blowing up the payload, so seems safe to just move forward.

  2. Is presets.redwoodVersion ever a thing? For sensativeCommand.positions, I wasn't as concerned with the type (undefined did not work 😢 ) but did want to make sure it was correct.

I'm fine to move forward if/when you think the same.

@cannikin
Copy link
Member

I'm not worried! As long as you're getting the duration numbers in there that you want I think we're good!

@thedavidprice thedavidprice merged commit 0998249 into main Feb 23, 2022
@thedavidprice thedavidprice deleted the dsp-benchmark-rw-test-duration branch February 23, 2022 00:45
@jtoar jtoar added this to the next-release milestone Feb 23, 2022
@thedavidprice thedavidprice modified the milestones: next-release, v0.47.0 Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

Benchmark rw test performance
3 participants