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.

Sign up for GitHub

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

Jump to bottom

test: unittest pnpm extension version resolution #1932

Closed
wants to merge 1 commit into from
Closed

test: unittest pnpm extension version resolution #1932

wants to merge 1 commit into from

Conversation

gzm0
Copy link
Contributor

@gzm0 gzm0 commented Oct 4, 2024

This is in preparation for #1907: As dependency resolution gets more complex, we should test it.

Note: No updates to to bzl_library targets, because it seems extensions.bzl isn't in a bzl_library.

Changes are visible to end-users: no

Test plan

  • Covered by existing test cases (e2e extension execution)
  • New test cases added

@aspect-workflows Aspect Workflows
Copy link

aspect-workflows bot commented Oct 4, 2024 •
edited
Loading

Test

All tests were cache hits

201 tests (100.0%) were fully cached saving 38s.

Test

e2e/bzlmod

All tests were cache hits

5 tests (100.0%) were fully cached saving 1s.

Test

e2e/gyp_no_install_script

All tests were cache hits

2 tests (100.0%) were fully cached saving 236ms.

Test

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 7s.

Test

e2e/npm_link_package

All tests were cache hits

3 tests (100.0%) were fully cached saving 839ms.

Test

e2e/npm_link_package-esm

All tests were cache hits

3 tests (100.0%) were fully cached saving 745ms.

Test

e2e/npm_translate_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 27ms.

Test

e2e/npm_translate_lock_empty

All tests were cache hits

1 test (100.0%) was fully cached saving 27ms.

Test

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 296ms.

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 112ms.

Test

e2e/npm_translate_lock_replace_packages

All tests were cache hits

3 tests (100.0%) were fully cached saving 716ms.

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 192ms.

Test

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 81ms.

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 81ms.

Test

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 713ms.

Test

e2e/pnpm_lockfiles

All tests were cache hits

40 tests (100.0%) were fully cached saving 3s.

Test

e2e/pnpm_workspace

All tests were cache hits

10 tests (100.0%) were fully cached saving 2s.

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

10 tests (100.0%) were fully cached saving 2s.

Test

e2e/repo_mapping

All tests were cache hits

2 tests (100.0%) were fully cached saving 233ms.

Test

e2e/rules_foo

All tests were cache hits

2 tests (100.0%) were fully cached saving 153ms.

Test

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 171ms.

Buildifier      Format

@gzm0
test: unittest pnpm extension version resolution
226ddba 
This is in preparation for aspect-build#1907: As dependency resolution gets more
complex, we should test it.

Note: No updates to to bzl_library targets, because it seems
`extensions.bzl` isn't in a bzl_library.
jbedard
jbedard reviewed Oct 4, 2024
View reviewed changes
npm/private/pnpm_extension.bzl
selected = versions[idx]
selected_tuple = _parse_version(selected)

result.notes.append("NOTE: repo '{}' has multiple versions {}; selected {}".format(name, versions, selected))
Copy link
Member

Choose a reason for hiding this comment

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

Why collect "notes" instead of just printing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we can test it easily (unless there is a mechanism in unittest to do that that I don't know about).

@jbedard jbedard mentioned this pull request Oct 4, 2024
Merged
@jbedard
Copy link
Member

jbedard commented Oct 5, 2024

@gzm0 I think this was all correctly merged in the other PR?

@jbedard jbedard closed this Oct 5, 2024
@gzm0
Copy link
Contributor Author

gzm0 commented Oct 5, 2024

Yep, looks like it (seems it got squashed, but the changes are there).

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

@jbedard jbedard jbedard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gzm0 @jbedard