SentinelArrays.jl icon indicating copy to clipboard operation
SentinelArrays.jl copied to clipboard

Resolve ambiguities

Open KeithWM opened this issue 2 years ago • 10 comments

Let me know what you think. I took the liberty of rewriting the tests using ReTest, as it allows for much faster iteration in development.

Maybe besides tests that only test the perceived problem of potential method ambiguities, I should also include some tests that test the actual result of the operations.

KeithWM avatar Mar 15 '23 22:03 KeithWM

Hmmm, why didn't CI run here?

quinnj avatar Apr 23 '23 05:04 quinnj

I assumed CI was not run because I am somehow not approved to be the trigger. It made sense to me that CI would not be triggered by a PR from some random contributor.

KeithWM avatar Apr 23 '23 07:04 KeithWM

Sorry for the slow response here. Yeah, it's weird though because usually it pops up a button to ask if I want to approve CI for a first-time contributor. But I don't see anyway to kick off a CI run here.

That said, here's my thoughts:

  • It feels hard to review this PR because of all the unrelated changes; I can't tell which tests are new and covering the changes because the entire files moved; it would be much easier to back out all the test file moving while we work thru the ambiguity issue and then do a follow up PR that's just about moving tests around
  • I think Aqua/JET have some ways to automatically test for ambiguities? I can't quite remember the details, but it could be nice to hook into something automated like that to try and avoid future issues.

quinnj avatar May 19 '23 06:05 quinnj

I have no idea why you aren't given the option to run the CI actions. Maybe Github knows this PR has a scope issue. ;-)

  • I realized after creating the PR that I made a mistake in adjusting the tests so fundamentally in a PR that also includes other changes. I will create a new PR including only the first commit of this one, then it's almost entirely a "move".
  • I was using Aqua.jl, that's how I found them. I could create an automated test for the ambiguities too, but that will fail until this PR (#86) is merged.

KeithWM avatar May 19 '23 21:05 KeithWM

Codecov Report

Patch coverage: 87.50% and project coverage change: +0.58 :tada:

Comparison is base (bc8aba3) 94.81% compared to head (8ea444c) 95.40%.

:exclamation: Current head 8ea444c differs from pull request most recent head b6af311. Consider uploading reports for the commit b6af311 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
+ Coverage   94.81%   95.40%   +0.58%     
==========================================
  Files           5        4       -1     
  Lines        1041     1000      -41     
==========================================
- Hits          987      954      -33     
+ Misses         54       46       -8     
Impacted Files Coverage Δ
src/chainedvector.jl 96.66% <87.50%> (+0.02%) :arrow_up:

... and 2 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar May 22 '23 22:05 codecov[bot]

Ok, finally got CI to run; can you take a look at the test failures? 1.3 is probably just too old, so we can bump that up to 1.6.

quinnj avatar May 22 '23 22:05 quinnj

I've made some new commits to expand code coverage.

KeithWM avatar May 23 '23 18:05 KeithWM

Resolve the conflicts and I think this is ready to go?

quinnj avatar Jun 03 '23 05:06 quinnj

I think I resolved the conflict. CI/CD should be run to confirm. (I should maybe have done a FF on my "main" rather than a merge commit from this main, but alas)

KeithWM avatar Jun 03 '23 15:06 KeithWM

Bump - is there anything holding this up?

nilshg avatar Nov 09 '23 14:11 nilshg