Skip to content

use /test/Project.toml instead of [extras] in /Project.toml#436

Merged
Krastanov merged 1 commit intomasterfrom
moderntoml
Aug 16, 2025
Merged

use /test/Project.toml instead of [extras] in /Project.toml#436
Krastanov merged 1 commit intomasterfrom
moderntoml

Conversation

@Krastanov
Copy link
Copy Markdown
Member

@Krastanov Krastanov commented Jun 3, 2025

This PR simplifies the Project.toml file by splitting off the test dependencies in a separate PR.

The main value of doing so is avoiding annoying compat bounds on test dependencies that leads to Graphs downgrading other packages on a user's system.

Before this PR the following was not possible ] add Graphs JuliaFormatter@2 because our tests had a dependencies on JuliaFormatter==1. Now that the test environment is separate this is not an issue.

Alternative to #433 and #435 that does not require changing anything about our formatting. Of course, switching to JuliaFormatter 2 would be nice at some point, but the urgency is to make sure that our package is not downgrading other people's environments.

@Krastanov Krastanov requested a review from simonschoelly June 3, 2025 02:31
@Krastanov Krastanov mentioned this pull request Jun 3, 2025
Closed
@Krastanov
Copy link
Copy Markdown
Member Author

Krastanov commented Jun 3, 2025

in #433 the concern of having Formatter for more than just tests was raised, e.g. if Formatter is used in a pre-commit hook: that is fine, as either way Formatter should not be a dependency of Graphs, rather just another package in the dev environment in which Graphs is already dev-ed (or in the root environment).

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 3, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.41%. Comparing base (c001dab) to head (97a299d).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #436   +/-   ##
=======================================
  Coverage   97.41%   97.41%           
=======================================
  Files         120      120           
  Lines        6953     6953           
=======================================
  Hits         6773     6773           
  Misses        180      180

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

simonschoelly
simonschoelly reviewed Jun 3, 2025
View reviewed changes
Comment thread .gitignore
Comment on lines -9 to -13
benchmark/Manifest.toml
.benchmarkci
*.cov
/Manifest.toml
/docs/Manifest.toml
Copy link
Copy Markdown
Member

@simonschoelly simonschoelly Jun 3, 2025

Choose a reason for hiding this comment

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

I assume that this means no Manifest.toml would get added to git?

Copy link
Copy Markdown
Member Author

@Krastanov Krastanov Jun 3, 2025

Choose a reason for hiding this comment

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

yes

@simonschoelly
Copy link
Copy Markdown
Member

simonschoelly commented Jun 3, 2025

I have some issues running the tests locally now - can you try if it works for you? I create open Julia v1.12 with a new temporary project: julia --project=temp then ]add Graphs and test Graphs. Then I see errors like the one below and after a while the tests hang:

Path from parents: Error During Test at /home/simon/.julia/dev/Graphs/test/shortestpaths/utils.jl:1
  Got exception outside of a @test
  ArgumentError: Package Graphs not found in current path, maybe you meant `import/using .Graphs`.
  - Otherwise, run `import Pkg; Pkg.add("Graphs")` to install the Graphs package.
  Stacktrace:
    [1] macro expansion
      @ ./loading.jl:2296 [inlined]
    [2] macro expansion
      @ ./lock.jl:273 [inlined]
    [3] __require(into::Module, mod::Symbol)
      @ Base ./loading.jl:2271
    [4] #invoke_in_world#3
      @ ./essentials.jl:1089 [inlined]
    [5] invoke_in_world
      @ ./essentials.jl:1086 [inlined]
    [6] require(into::Module, mod::Symbol)
      @ Base ./loading.jl:2260
    [7] macro expansion
      @ ~/.julia/dev/Graphs/test/shortestpaths/utils.jl:2 [inlined]
    [8] macro expansion
      @ ~/.julia/juliaup/julia-1.11.5+0.x64.linux.gnu/share/julia/stdlib/v1.11/Test/src/Test.jl:1704 [inlined]
    [9] top-level scope
      @ ~/.julia/dev/Graphs/test/shortestpaths/utils.jl:2
   [10] include(fname::String)
      @ Main ./sysimg.jl:38
   [11] macro expansion
      @ ~/.julia/dev/Graphs/test/runtests.jl:177 [inlined]
   [12] macro expansion
      @ ~/.julia/juliaup/julia-1.11.5+0.x64.linux.gnu/share/julia/stdlib/v1.11/Test/src/Test.jl:1704 [inlined]
   [13] macro expansion
      @ ~/.julia/dev/Graphs/test/runtests.jl:175 [inlined]
   [14] macro expansion
      @ ~/.julia/juliaup/julia-1.11.5+0.x64.linux.gnu/share/julia/stdlib/v1.11/Test/src/Test.jl:1704 [inlined]
   [15] top-level scope
      @ ~/.julia/dev/Graphs/test/runtests.jl:154
   [16] include(fname::String)
      @ Main ./sysimg.jl:38
   [17] top-level scope
      @ none:6
   [18] eval
      @ ./boot.jl:430 [inlined]
   [19] exec_options(opts::Base.JLOptions)
      @ Base ./client.jl:296
   [20] _start()
      @ Base ./client.jl:531
Actual tests: Error During Test at /home/simon/.julia/dev/Graphs/test/runtests.jl:174
  Got exception outside of a @test
  LoadError: ArgumentError: Package Graphs not found in current path, maybe you meant `import/using .Graphs`.
  - Otherwise, run `import Pkg; Pkg.add("Graphs")` to install the Graphs package.
  Stacktrace:

@Krastanov
Copy link
Copy Markdown
Member Author

Krastanov commented Jun 3, 2025

The following worked for me (maybe the difference is using add with(out) specifying the branch of the repo -- there are some differences between add package_name and add package_repo with respect to tags and branches that I have not tried to understand):

> julia +1.12 --project=an_empty_folder -tauto,auto
pkg> add Graphs#moderntoml
pkg> test Graphs
... the usual output

@Krastanov Krastanov requested a review from simonschoelly June 6, 2025 12:48
@Krastanov
Copy link
Copy Markdown
Member Author

Krastanov commented Jul 2, 2025

@simonschoelly , could you check this is still causing you issues locally? Probably the issue was just a missing ] resolve after the modification of your local Project.toml (it should not happen in a new environment or a resolved old environment)

@Krastanov Krastanov mentioned this pull request Aug 5, 2025
Closed
@gdalle
Copy link
Copy Markdown
Member

gdalle commented Aug 16, 2025

Works for me locally on 1.11

@Krastanov
Copy link
Copy Markdown
Member Author

Krastanov commented Aug 16, 2025

Closing and reopening to rerun the CI. I will merge after that if there are no failures.

@Krastanov Krastanov closed this Aug 16, 2025
@Krastanov Krastanov reopened this Aug 16, 2025
@Krastanov Krastanov merged commit 769e50d into master Aug 16, 2025
22 checks passed
@Krastanov Krastanov deleted the moderntoml branch August 16, 2025 20:30
@Krastanov Krastanov mentioned this pull request Aug 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simonschoelly simonschoelly Awaiting requested review from simonschoelly

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Krastanov @simonschoelly @gdalle