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

WIP: GenerateCoverage Experiment #718

Open
wants to merge 432 commits into
base: vara-dev
Choose a base branch
from
Open

WIP: GenerateCoverage Experiment #718

wants to merge 432 commits into from

Conversation

danjujan
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Attention: 472 lines in your changes are missing coverage. Please review.

Comparison is base (3fa246b) 68.55% compared to head (6def304) 69.10%.
Report is 1 commits behind head on vara-dev.

Additional details and impacted files
@@             Coverage Diff              @@
##           vara-dev     #718      +/-   ##
============================================
+ Coverage     68.55%   69.10%   +0.54%     
============================================
  Files           338      344       +6     
  Lines         26434    28639    +2205     
============================================
+ Hits          18122    19791    +1669     
- Misses         8312     8848     +536     
Files Coverage Δ
tests/experiment/test_workload_util.py 100.00% <100.00%> (ø)
tests/utils/test_doc_util.py 100.00% <ø> (ø)
varats-core/varats/experiment/workload_util.py 92.85% <100.00%> (+0.21%) ⬆️
varats-core/varats/project/varats_command.py 83.01% <100.00%> (+1.38%) ⬆️
.../varats/provider/feature/feature_model_provider.py 82.85% <100.00%> (ø)
varats-core/varats/utils/git_util.py 85.78% <100.00%> (ø)
varats/varats/projects/c_projects/bzip2.py 50.00% <100.00%> (ø)
varats/varats/projects/c_projects/gzip.py 61.40% <100.00%> (+1.40%) ⬆️
varats/varats/projects/c_projects/picosat.py 59.25% <100.00%> (+1.56%) ⬆️
varats/varats/projects/c_projects/xz.py 66.03% <100.00%> (ø)
... and 8 more

... and 10 files with indirect coverage changes

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

@danjujan danjujan requested a review from vulder January 11, 2023 09:11
@danjujan danjujan marked this pull request as ready for review April 5, 2023 09:04
Copy link
Contributor

@vulder vulder left a comment

Choose a reason for hiding this comment

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

Hmm, I'm not 100% sure about the zip files for the tests. On the one side, it's good for testing, on the other, we don't know how to update/fix them when stuff breaks.
Maybe, we should at least put some doc/wording in a file that specifies how the information can be regenerated?

  • I hope, this does not make the tests brittel

@boehmseb what's your take on that?

Overall, the implementation already looks quite good.

tests/helper_utils.py Outdated Show resolved Hide resolved

class TestCodeRegion(unittest.TestCase):

def test_feature_config_report_map(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this tests, the beginning looks a bit like testing the config handling part of the configuration, which should be tested in separation and in the config module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only tests if ConfigCoverageReportMapping will use the right configs for diffing.

@@ -0,0 +1,708 @@
"""Code region tree and coverage report."""

from __future__ import annotations
Copy link
Contributor

Choose a reason for hiding this comment

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

From which python version is this? As we have update to min 3.9 we maybe don't need this anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is still necessary. Otherwise the following is not possible. See https://peps.python.org/pep-0563/ and https://peps.python.org/pep-0649/

class CodeRegion
    def from(cls, x) -> CodeRegion:
       ...

varats/varats/data/reports/llvm_coverage_report.py Outdated Show resolved Hide resolved
varats/varats/data/reports/llvm_coverage_report.py Outdated Show resolved Hide resolved
varats/varats/data/reports/llvm_coverage_report.py Outdated Show resolved Hide resolved
varats/varats/data/reports/llvm_coverage_report.py Outdated Show resolved Hide resolved
varats/varats/experiments/vara/llvm_coverage_experiment.py Outdated Show resolved Hide resolved
varats/varats/experiments/vara/llvm_coverage_experiment.py Outdated Show resolved Hide resolved
return repr(self.x)


class RunConfig(tp.FrozenSet[tp.Tuple[str, ConfigValue]]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to build a config class here, is the one that parses the config format not working. If not, can we maybe adapt it?

@boehmseb
Copy link
Member

boehmseb commented Apr 6, 2023

Hmm, I'm not 100% sure about the zip files for the tests. On the one side, it's good for testing, on the other, we don't know how to update/fix them when stuff breaks. Maybe, we should at least put some doc/wording in a file that specifies how the information can be regenerated?

* I hope, this does not make the tests brittle

@boehmseb what's your take on that?

We kind of brought this on ourselves by introducing .zip files as reports.
The corresponding report class should make obvious why there is a .zip lying around.
So technically, this is no different to including a normal report as test input.
I understand your concern, but I'm not sure whether an additional readme of some sorts can provide additional information that is not already encoded by the test and the report class.

@danjujan
Copy link
Contributor Author

Hmm, I'm not 100% sure about the zip files for the tests. On the one side, it's good for testing, on the other, we don't know how to update/fix them when stuff breaks. Maybe, we should at least put some doc/wording in a file that specifies how the information can be regenerated?

* I hope, this does not make the tests brittle

@boehmseb what's your take on that?

We kind of brought this on ourselves by introducing .zip files as reports. The corresponding report class should make obvious why there is a .zip lying around. So technically, this is no different to including a normal report as test input. I understand your concern, but I'm not sure whether an additional readme of some sorts can provide additional information that is not already encoded by the test and the report class.

As @boehmseb said. The zip files were obtained by copy pasting them from the results folder after running the GenerateCoverage experiment.

@vulder
Copy link
Contributor

vulder commented Apr 13, 2023

That part I know and "buy", I'm more concerned to how we can update them if needed. We should add docu to the tests how to regenerate the files, project + experiment.
But what also concerns me, it looks like some of the paths where edited by had, which could introduce brittleness as this rewriting is not done automatically. I would be fine with the zips if we have a small description in the test files that tells a developer how to regenerate these files, because from what I see, it's a bit more difficult than "just" running the experiment.

By the way, as soon as we have a "docu format" or idea how to specify these, we should probably also document the other ones.

@danjujan
Copy link
Contributor Author

That part I know and "buy", I'm more concerned to how we can update them if needed. We should add docu to the tests how to regenerate the files, project + experiment. But what also concerns me, it looks like some of the paths where edited by had, which could introduce brittleness as this rewriting is not done automatically. I would be fine with the zips if we have a small description in the test files that tells a developer how to regenerate these files, because from what I see, it's a bit more difficult than "just" running the experiment.

By the way, as soon as we have a "docu format" or idea how to specify these, we should probably also document the other ones.

I regenerated the files and noted down my steps. https://github.com/se-sic/VaRA-Tool-Suite/blob/fba20d72770514086ea65262a491378c5ea11963/tests/TEST_INPUTS/results/FeaturePerfCSCollection/HowToReproduce.md. Feedback welcome.

@danjujan danjujan mentioned this pull request May 2, 2023
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