Move service tests alongside code #1563
Conversation
paulmelnikow
added
developer-experience
scripts/reorg.fish
Outdated
| @@ -0,0 +1,20 @@ | |||
| #!/usr/bin/env fish | |||
There was a problem hiding this comment.
We don't necessarily need to check this in at the end, though it may be useful as we iterate.
Generated by 🚫 dangerJS |
|
Would love to get this merged to avoid merge conflicts in new work. @chris48s Is it looking okay to you? |
# Conflicts: # services/json/json.tester.js # services/xml/xml.tester.js
There was a problem hiding this comment.
Sorry. I've been meaning to have a look at this one but I have been putting it off because the diff is huge. Fortunately it is mainly moving stuff about :)
Broadly this seems like the right change but I have left a couple of comments.
Once this is merged, there are probably a few open PRs that will need to be rebased to account for this, but I don't think there will be too many. Worth doing a quick sweep afterwards to ping the authors.
I also ran the full suite of service tests. Looks like there are no broken imports which is what I was mainly checking for, but it also looks like there are a few flaky service tests which need a bit of love. Lets not get bogged down in fixing them here. I'll raise an issue for it.
| @@ -97,7 +97,7 @@ boilerplate: | |||
| 'use strict'; | |||
There was a problem hiding this comment.
A few lines up from here (line 92) there is a path service-tests/travis.js which needs updating, but annoyingly I can't leave a diff comment on that exact line
scripts/reorg.fish
Outdated
| mv $file services/$service/$service.tester.js | ||
| end | ||
|
|
||
| mv service-tests/helpers/nuget-fixtures.js services/nuget/nuget-fixtures.js |
There was a problem hiding this comment.
I'm not sure I have an obviously better suggestion here, but having stuff like this and test-validators.js in the root of the services dir feels like maybe conceptually the wrong place to put them.. I get that making a subdir for them then makes that subdir behave like a 'service', which is also wrong. Maybe there is a case for /lib/service or /lib/testing ?
The correct answer to that may only really become clear once we start feeling our way around some of the issues we're discussing in #1358 (comment)
There was a problem hiding this comment.
lib/testing seems like a great name. lib/service seems a little ambiguous. Does it contain service-related helpers (perhaps including base.js) or helpers for particular types of external services? Maybe lib/service-specific would be clearer.
Yea… service/lib or service/common isn't quite right either. It might invite people to misclassify helpers there which ought to be placed in lib/.
Though… huh. Maybe that's just backward thinking. Maybe all the helper functions designed to be used from the services should be moved from lib/ into services/common. Or services/COMMON which sorts at the top. They certainly would be easier to find. I'm thinking of the text formatters, error helpers, and the version and license helpers. Honestly that's most of what's in lib/. What's left would basically be "core lib".
Thoughts?
I know! It's a big diff. Thanks for wading in! And thanks for running the test suite! Makes sense what you're saying about pinging authors. For what it's worth, I find changes like these easier to feel out at rest, by checking out the branch. In stock, not flow. |
# Conflicts: # services/base.js
|
Since the bulk of this is in good shape and it's a big step in the right direction, I'll merge it to avoid it going stale again, and because the small change will be easier to review. |
Per discussion in #1543
This isn't completely working yet, though I thought I'd open it for comments before I put a lot of work into resolving all the module paths.Would be great if folks could play around with the ergonomics of this branch. Does it work well for you?