Skip to content
This repository was archived by the owner on Jun 11, 2020. It is now read-only.

Conversation

@jablko
Copy link
Contributor

@jablko jablko commented May 8, 2020

If package A has a test dependency on B (because A's tests import B) and A has a path mapping B -> B/v#, then the compiler will use older-version B/v#, but AllPackages.allDependencyTypings() currently returns only the latest-versions of test dependencies:

for (const name of pkg.testDependencies) {
const versions = this.data.get(getMangledNameForScopedPackage(name));
if (versions) {
yield versions.getLatest();

This comes up in check-parse-results.ts checkPathMappings(), which checks for unused path mappings: All of a package's path mappings must be accounted for by a transitive dependency.

If A has a test dependency on B and a path mapping B -> B/v#, and B/v# in turn depends on C/v#, then A needs path mapping C -> C/v# or A's tests will combine possibly-incompatible packages B/v# and C-latest. But the compiler and checkPathMappings() currently use different versions of B: Unless B-latest also depends on C/v#, checkPathMappings() will complain that A's C -> C/v# path mapping is unused. Then A can't get past the CI automation: Its tests fail without the path mapping and checkPathMappings() fails with it.

checkPathMappings() already handles the case that A has a regular dependency on B. It respects the path mapping and uses the older version B/v#, same as the compiler. This is implemented in definition-parser.ts calculateDependencies():

dependencies.push({ name: dependencyName, version: pathMappingVersion });

This PR applies the same logic to AllPackages.allDependencyTypings() and test dependencies.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great fix. I've noticed this before and was very puzzled.

I have the same request as your other PR to move this fix and add a test by adding to createMockDT.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants