Skip to content

using is_fixed - #28

Merged
oxinabox merged 9 commits into
JuliaTesting:release-1.4from
willtebbutt:wct/is-fixed
Aug 11, 2021
Merged

using is_fixed#28
oxinabox merged 9 commits into
JuliaTesting:release-1.4from
willtebbutt:wct/is-fixed

Conversation

@willtebbutt

@willtebbutt willtebbutt commented Aug 10, 2021

Copy link
Copy Markdown

As per @oxinabox 's request in #26 .

I'm not sure how to test this as I'm not entirely sure what the problem was in the first place 🤷

@oxinabox

Copy link
Copy Markdown
Member

Hopefully you can test it by adding what ever package triggered the problem as a test time dependency,
and then testing it much like the other tests in test/activate_set.jl

@willtebbutt

Copy link
Copy Markdown
Author

Alas, the version of ChainRulesCore required in the test deps and KernelFunctions aren't yet compatible. Any other suggestions?

@oxinabox

Copy link
Copy Markdown
Member

Change the version of ChainRulesCore to one that is compatible.
It just needs to be a version that has the same test time dependencies that we are checking for.
and then we lock it inplace with a ="=1.0.2" to be sure.

@willtebbutt

Copy link
Copy Markdown
Author

Okay -- I've not restricted CRC to 0.10.13, which is compatible with KernelFunctions 0.10.12. Is this alright? I'm still not really clear on how to know if this is a permissible version of CRC.

@codecov-commenter

codecov-commenter commented Aug 11, 2021

Copy link
Copy Markdown

Codecov Report

Merging #28 (5f24a02) into release-1.4 (e1af8ff) will increase coverage by 0.73%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           release-1.4      #28      +/-   ##
===============================================
+ Coverage        78.75%   79.48%   +0.73%     
===============================================
  Files                4        3       -1     
  Lines               80       78       -2     
===============================================
- Hits                63       62       -1     
+ Misses              17       16       -1     
Impacted Files Coverage Δ
src/common.jl 77.41% <0.00%> (+2.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1af8ff...5f24a02. Read the comment docs.

@willtebbutt

Copy link
Copy Markdown
Author

Change of plan -- I've added a laughably simple test to prevent reversion, because I was unable to find a version of KernelFunctions which played nicely with ChainRulesCore on 1.4, 1.5, and 1.6, nor was I able to really tie down the cause of hitting that particular branch.

Comment thread test/activate_set.jl Outdated
push!(LOAD_PATH, mktempdir()) # put something weird in LOAD_PATH for testing
orig_load_path = Base.LOAD_PATH
try
TestEnv.activate("KernelFunctions")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
TestEnv.activate("KernelFunctions")
TestEnv.activate("KernelFunctions")
@test true # would have errored before this point if this failed.

Comment thread test/activate_set.jl Outdated
end
end

# See #26

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer full URLs for comments so I can click them to go the other code

Suggested change
# See #26
# See https://github.com/JuliaTesting/TestEnv.jl/issues/26

Comment thread test/activate_set.jl Outdated
end

# See #26
@testset "KernelFunctions" begin

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
@testset "KernelFunctions" begin
@testset "KernelFunctions: is_fixed issue" begin

@oxinabox

Copy link
Copy Markdown
Member

I'm still not really clear on how to know if this is a permissible version of CRC.

It just needs to have the same test-time dependencies.

Comment thread test/activate_set.jl
end

# https://github.com/JuliaTesting/TestEnv.jl/issues/26
@test isdefined(TestEnv, :isfixed)

@oxinabox oxinabox Aug 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Welp, I guess that works.
Feels unsatisfying, since it is testing implementation rather than problem, though.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Completely agree. Presumably the fact that it isn't hit in the existing tests means that a chunk of code doesn't get tested :(

@oxinabox

Copy link
Copy Markdown
Member

lets merge this and later worry about finding a better test

@oxinabox
oxinabox merged commit 7a329a2 into JuliaTesting:release-1.4 Aug 11, 2021
@willtebbutt
willtebbutt deleted the wct/is-fixed branch August 11, 2021 18:05
This was referenced Aug 11, 2021
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.

3 participants