Simplify monorepo-workflow-operations #21
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcmire
force-pushed
the
simplify-monorepo-workflow-tests
branch
from
f975b68 to
a87fb54
Compare
mcmire
commented
Jul 22, 2022
mcmire
force-pushed
the
fix-written-manifests
branch
from
e096a90 to
e8ccf1a
Compare
mcmire
force-pushed
the
simplify-monorepo-workflow-tests
branch
3 times, most recently
from
be53bb8 to
3db4897
Compare
mcmire
changed the title
mcmire
commented
Aug 22, 2022
| if (versionSpecifier instanceof SemVer) { | ||
| const comparison = versionSpecifier.compare(currentVersion); | ||
|
|
||
| if (comparison === 0) { |
There was a problem hiding this comment.
I've moved these checks to when the release spec is validated, as it makes more sense there.
| @@ -32,6 +36,11 @@ export async function determineInitialParameters( | |||
| project.rootPackage.validatedManifest.name.replace('/', '__'), | |||
| ) | |||
| : path.resolve(cwd, inputs.tempDirectory); | |||
| const parsedTodayTimestamp = | |||
There was a problem hiding this comment.
This is moved from monorepo-workflow-operations, where it was done on the fly.
| * @param args.today - The current date. | ||
| * @returns A promise for information about the new release. | ||
| */ | ||
| export async function planRelease({ |
There was a problem hiding this comment.
This whole file was moved from monorepo-workflow-operations.
| const newVersion = | ||
| versionSpecifier instanceof SemVer | ||
| ? versionSpecifier | ||
| : new SemVer(currentVersion.toString()).inc(versionSpecifier); |
There was a problem hiding this comment.
The checks that used to be here around the new version (formerly in monorepo-workflow-operations.ts) have been moved to validateReleaseSpecification in release-specification.ts.
There was a problem hiding this comment.
Good idea, that makes much more sense
| * @param releaseName - The name of the release, which will be used to name the | ||
| * commit and the branch. | ||
| */ | ||
| export async function captureChangesInReleaseBranch( |
There was a problem hiding this comment.
This got moved from workflow-operations.ts, as it really pertains to a repo.
| * (optional) and the properties that will go into the object (optional). | ||
| * @returns The mock Package object. | ||
| */ | ||
| export function buildMockMonorepoRootPackage( |
There was a problem hiding this comment.
This is now no longer used entirely.
The tests for `monorepo-workflow-operations.ts` are not as readable or maintainable as I'd like them to be. This commit attempts to fix that. * While there isn't a whole lot we can do in terms of the scenarios we're testing because the function makes a lot of calls and the logic inside of it is somewhat complicated, we can at least move code that is responsible for parsing the TODAY environment variable, building a ReleasePlan object from the release spec, and applying the updates to the repos themselves out into separate files. This simplifies the test data and removes a few tests entirely. * We also simplify the structure of the tests so that we aren't using so many nested `describe` blocks. This ends up being very difficult to keep straight, so the flattened layout here makes it a little more palatable. * We also simplify the setup code for each test. Currently we are mocking all of the dependencies for `followMonorepoWorkflow` in one go, but we're doing so in a way that forces the reader to wade through a bunch of type definitions. That isn't really that helpful. The most complicated part of reading the tests for `followMonorepoWorkflow` isn't the dependencies — it's the logic. So we take all of the decision points we have to make in the implementation and represent those as options to our setup function in the tests so it's as clear as possible which exact scenario is being tested just by reading the test. * Finally while we're moving things around, we can also move the check we perform to ensure that the new version of a package is greater than its existing version so that it is earlier in the entire workflow, i.e. from building the release plan to the validating the release specification. This also reduces the number of tests we have to write.
mcmire
force-pushed
the
simplify-monorepo-workflow-tests
branch
from
3db4897 to
c30ce5c
Compare
Gudahtt
reviewed
Aug 24, 2022
Gudahtt
reviewed
Aug 24, 2022
Gudahtt
reviewed
Aug 24, 2022
Gudahtt
reviewed
Aug 24, 2022
Gudahtt
reviewed
Aug 24, 2022
Gudahtt
reviewed
Aug 24, 2022
Gudahtt
reviewed
Aug 24, 2022
Gudahtt
reviewed
Aug 24, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The tests for
monorepo-workflow-operations.tsare not as readable ormaintainable as I'd like them to be. This commit attempts to fix that.
we're testing because the function makes a lot of calls and the logic
inside of it is somewhat complicated, we can at least move
code that is responsible for parsing the TODAY environment variable,
building a ReleasePlan object from the release spec, and applying the
updates to the repos themselves out into separate files. This
simplifies the test data and removes a few tests entirely.
many nested
describeblocks. This ends up being very difficult tokeep straight, so the flattened layout here makes it a little more
palatable.
mocking all of the dependencies for
followMonorepoWorkflowin onego, but we're doing so in a way that forces the reader to wade through
a bunch of type definitions. That isn't really that helpful. The most
complicated part of reading the tests for
followMonorepoWorkflowisn't the dependencies — it's the logic. So we take all of the
decision points we have to make in the implementation and represent
those as options to our setup function in the tests so it's as clear
as possible which exact scenario is being tested just by reading the
test.
we perform to ensure that the new version of a package is greater than
its existing version so that it is earlier in the entire workflow,
i.e. from building the release plan to the validating the release
specification. This also reduces the number of tests we have to write.