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

Unify statistics computation and access to coverage data #837

Merged
merged 7 commits into from
Jan 26, 2020
Merged

Unify statistics computation and access to coverage data #837

merged 7 commits into from
Jan 26, 2020

Conversation

PragTob
Copy link
Collaborator

@PragTob PragTob commented Jan 25, 2020 •
edited
Loading

The bigger purpose is multi step where the access to coverage data is normalized and hence methods that work with expecting a minimum amount of coverage and formatters can work with the known good same data base as opposed to memoizing all different method names.

This is good enough as an intermediate PR.

By virtue of unifying coverage computation we also get rid of inconsistencies like #565

While I was there and happily modified things, I also got rid off inheriting from a core class (Array).

  • missing unit tests for my new class (but everything should still be tested through the other tests that did this anyhow) - normally I TDD but here I was anxious/unsure how the end result would look like/work (and it indeed changed purpose/scope multiple time)

@PragTob
Introduce a more general appraoch to determining the result
7925f9d 
Use custom CoverageData, centralizing some common computation
as well.
@PragTob
Stop inheriting from core classes
80e10a9 
Imo/afaik this is an anti pattern, especially when combined with
overriding initialize as due to optimizations your initialize
might be skipped, which is of course kinda bad.

This should retain all the functionality while not running into
potentially dangerous territory.
@PragTob
Remove covered_strength rounding
6eef92c 
It was the only non output place in the entire code base where
we rounded it as best as I can tell. Rounding can be done in
formatters (imo) and by doing it on the source file level
we lost precision in the accumulation for all files.

Went back all the way to #59 and couldn't see a reason why
it _had_ to be rounded. Also, no tests fail so 🤷
@PragTob
Use CoverageData everywhere for consistency
85ece5e 
This allows us to get rid off duplicated and inconsistent
calculations. As a side effect of this, #565 is fixed now.

So, fixes #565.

The bigger purpose is multi step though where the access
to coverage data is normalized and hence methods that work
with expecting a minimum amount of coverage and formatters
can work with the known good same data base as opposed
to memoizing all different method names.

At first I wanted to only do it for FileList and leave the
rest for later. But the strength computation needed the
total number of lines of code and so to pass it in I'd have
had to calculate it but I wanted to calculate it only in
CoverageData and there the avalanche started....

Commit also includes moving methods around in SourceFile to be
private - it happened in the moment, forgive me for no extra
commit (it was hard to determine what I could delete/what I need
to adjust etc.).
@PragTob
Memoize total_branches
66147a9 
I'm not super happy with this function in total (it's mostly used
for counting/size and we could use coverage data now) but I don't
want to break interface as much right here. So for compatibility
this helps it not create arrays all over the place all the time.
@PragTob
Copy link
Collaborator Author

PragTob commented Jan 25, 2020

No idea why the warning production/check hadn't worked for me locally in the rake task 🤔

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 25, 2020

Oh wow yeah, of course force pushing deletes references to comments. Sorry @krtschmr wanted to keep a relatively nice history 😅

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 25, 2020

Seems like a flaky we should fix - @deivid-rodriguez was this the one you saw before?


expected to find visible css ".header h3" with text "lib/faked_project/some_class.rb" but there were no matches. Also found "lib/faked_project/framework_specific.rb", "lib/faked_project/meta_magic.rb", "lib/faked_project/some_class.rb", "spec/forking_spec.rb", "spec/meta_magic_spec.rb", "spec/some_class_spec.rb", "lib/faked_project.rb", which matched the selector but not all filters.  (RSpec::Expectations::ExpectationNotMetError)
./features/step_definitions/html_steps.rb:63:in `/^I open the detailed view for "(.+)"$/'
features/branch_coverage.feature:30:in `When I open the detailed view for "lib/faked_project/some_class.rb"'

Failing Scenarios:
cucumber features/branch_coverage.feature:6 # Scenario:

@deivid-rodriguez
Copy link
Collaborator

Nop, that's a different one. Mine was about timeouts in apparition while opening coverage reports.

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 26, 2020

ugh, that one would seem hard to solve. Damn :(

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 26, 2020 

 expected to find visible css ".header h3" with text "lib/faked_project/some_class.rb" but there were no matches. Also found "lib/faked_project/framework_specific.rb", "lib/faked_project/meta_magic.rb", "lib/faked_project/some_class.rb", "spec/forking_spec.rb", "spec/meta_magic_spec.rb", "spec/some_class_spec.rb", "lib/faked_project.rb", which matched the selector but not all filters.  (RSpec::Expectations::ExpectationNotMetError)
./features/step_definitions/html_steps.rb:63:in `/^I open the detailed view for "(.+)"$/'
features/branch_coverage.feature:30:in `When I open the detailed view for "lib/faked_project/some_class.rb"'

Same flaky again :'(

@PragTob
Copy link
Collaborator Author

PragTob commented Jan 26, 2020

And in a scenario I wrote again 😱 And I'm the one who always claims I'm sadly proficient at debugging flakies - makes me think it's that way because I write so many 😢

@PragTob PragTob merged commit 86c0f27 into master Jan 26, 2020
@PragTob PragTob deleted the result-determination branch January 26, 2020 19:18
@deivid-rodriguez
Copy link
Collaborator

You're not alone @PragTob :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@PragTob @deivid-rodriguez