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

Use the RegistryInstances API in get_latest_version! and get_existing_registries! #449

Merged
merged 5 commits into from
Mar 14, 2023
Merged

Use the RegistryInstances API in get_latest_version! and get_existing_registries! #449

merged 5 commits into from
Mar 14, 2023

Conversation

andreasnoack
Copy link
Contributor

@andreasnoack andreasnoack commented Feb 9, 2023 •
edited by DilumAluthge
Loading

to make use_existing_registries work on more recent versions of Julia.

We use RegistryInstances to avoid using Pkg internals.

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 9, 2023 •
edited
Loading

Instead of using Pkg internals, can you use RegistryInstances.jl?

EDIT: This PR now uses RegistryInstances.jl (instead of Pkg internals).

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 9, 2023 •
edited
Loading

We'll also need a [compat] entry for RegistryInstances?

EDIT: We now have a [compat] entry for RegistryInstances.

DilumAluthge
DilumAluthge reviewed Feb 9, 2023
View reviewed changes
DilumAluthge
DilumAluthge reviewed Feb 9, 2023
View reviewed changes
@andreasnoack
Use Pkg's API in get_latest_version! and get_existing_registries!
5ed2477 
to make use_existing_registries work on more recent versions of
Julia.
@andreasnoack
Copy link
Contributor Author

andreasnoack commented Feb 10, 2023 •
edited
Loading

Aren't

CompatHelper.jl/test/dependencies.jl

Lines 40 to 48 in cb50eed

CompatHelper.clone_all_registries([
Pkg.RegistrySpec(; name=registry_1_name, url=""),
Pkg.RegistrySpec(; name=registry_2_name, url=""),
]) do resp
@test length(resp) == 2
@test contains(resp[1], registry_1_name)
@test contains(resp[2], registry_2_name)
end
pseudo tests in that they don't actually check if there are cloned registries? Isn't the check in the current form just that you can push string to a vector? I tried to change clone_all_registries to use reachable_registries instead building a vector of paths but reachable_registries returns an empty list because git_clone doesn't seem to actually clone any registries. At least that's how I read the failure in https://github.com/JuliaRegistries/CompatHelper.jl/actions/runs/4144273670/jobs/7167123638#step:17:923

Is there a way to use real registries here?

@DilumAluthge DilumAluthge changed the title Use Pkg's API in get_latest_version! and get_existing_registries! Use the RegistryInstances API in get_latest_version! and get_existing_registries! Feb 10, 2023
@DilumAluthge
Copy link
Member

Broadly speaking, we have two sets of tests here: the unit tests and the integration tests. The unit tests frequently use mocking (via Mocking.jl). In contrast, the integration tests use no mocking, and thus are always using real resources.

In the unit test you've linked, it looks like we mock the "git clone" functionality. So in its current state, this test is only testing the interface of function, not the behavior itself. The interface will be tested in the integration tests, but it's not always possible to cover all code paths in the integration tests (and we require 100% line coverage), so mocked unit tests can help hit other code paths.

For this specific unit test, yeah, I think it's fine to switch it from being mocked to not-mocked. You'd first modify

CompatHelper.jl/test/dependencies.jl

Line 39 in cb50eed

apply([mktempdir_patch, git_clone_patch]) do
- for this test, we likely need to stop applying one or both of those Mocking.jl patches. Then, for the test, I think you'll need to do something like:

  1. Save the original value of Base.DEPOT_PATH, etc.
  2. mktempdir()` a new temp depot
  3. Use Pkg to install more than one registry in that temp depot
  4. Run the desired tests with that temp depot
  5. Restore the original values of Base.DEPOT_PATH, etc.

@DilumAluthge
Copy link
Member

As far as where to get the multiple registries:

  1. General
  2. Maybe https://github.com/JuliaRegistries/Test?

@andreasnoack andreasnoack force-pushed the master branch 14 times, most recently from 8f20744 to a7eab6b Compare March 14, 2023 07:09
@andreasnoack
Copy link
Contributor Author

@DilumAluthge I finally managed to get tests passing here.

@DilumAluthge
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Mar 14, 2023
@bors
Try #449:
37094c8
@bors
Copy link
Contributor

bors bot commented Mar 14, 2023

try

Build succeeded:

@DilumAluthge
Copy link
Member

Can you bump the minor version number?

@andreasnoack
Copy link
Contributor Author

Done

@DilumAluthge
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Mar 14, 2023

Build succeeded:

@bors bors bot merged commit a477046 into JuliaRegistries:master Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DilumAluthge DilumAluthge DilumAluthge 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
@andreasnoack @DilumAluthge