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.

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

Use TestEnv #89

Merged
merged 7 commits into from
Oct 18, 2024
Merged

Use TestEnv #89

merged 7 commits into from
Oct 18, 2024

Conversation

mmiller-max
Copy link
Member

@mmiller-max mmiller-max commented Sep 23, 2022

I've just merged in main and this seems to be passing, picking up my work from over two years ago! Unfortunately I don't remember why this wasn't working before or why I wasn't happy with it, possibly because it failed for V1 (but then I dropped tests for V1). Would be grateful for @oxinabox and/or @omus to take a look at this 🙏
Hopefully will fix a few issues (e.g. #121 #122 )

edit: Just added back in V1 tests so will see what happens
edit 2: They fail, and they fail on master too (at least locally for me). At this point I think the best way forward is to remove 1.0 unless anyone has any objections?

@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #89 (b80d707) into master (8ba6874) will decrease coverage by 2.55%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   96.24%   93.69%   -2.56%     
==========================================
  Files           7        7              
  Lines         533      460      -73     
==========================================
- Hits          513      431      -82     
- Misses         20       29       +9     
Impacted Files Coverage Δ
src/TestReports.jl 100.00% <ø> (ø)
src/runner.jl 97.39% <100.00%> (+1.15%) ⬆️
src/compat_check.jl 74.71% <0.00%> (-15.18%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@oxinabox oxinabox mentioned this pull request May 22, 2024
# Conflicts:
#	src/TestReports.jl
#	src/runner.jl
@mmiller-max mmiller-max marked this pull request as ready for review October 15, 2024 20:55
src/runner.jl Outdated
testfilepath = gettestfilepath(ctx, pkgspec)

ctx, pkgspec = try
TestEnv.ctx_and_pkgspec(pkg)
Copy link
Member

Choose a reason for hiding this comment

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

ideally we wouldn't use the TestEnv internals here.
Because we would like to contain all the evil of accessing interals of Pkg into TestEnv so TestReports can be safe from it

Copy link
Member

Choose a reason for hiding this comment

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

but i don't think should block on this

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a TODO and will make an issue after merge

Copy link
Member

@oxinabox oxinabox 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 clearly a solid improvement

@mmiller-max mmiller-max merged commit 81ad054 into master Oct 18, 2024
8 checks passed
@mmiller-max mmiller-max deleted the mm/use-test-env branch October 18, 2024 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants