Run mutation tests in CI for changed files#1364
Run mutation tests in CI for changed files#1364souradeep-das wants to merge 4 commits intoethereum:forks/osakafrom
Conversation
.github/workflows/test.yaml
Outdated
| run: pip install mutmut==3.3.0 | ||
| - name: Apply mutmut patch | ||
| run: | | ||
| git clone https://github.com/souradeep-das/mutate-execution-specs.git |
There was a problem hiding this comment.
should i copy the patch to this repo instead?
There was a problem hiding this comment.
Instead of patching, you can install a git dependency:
pip install git+https://github.com/souradeep-das/mutmut.git@<commit hash>I think that would be easier to maintain than a patch file.
On a separate note, would something like this work (and maybe we can upstream)?
diff --git a/mutmut/__main__.py b/mutmut/__main__.py
index 0fb1ad9..15b16ef 100644
--- a/mutmut/__main__.py
+++ b/mutmut/__main__.py
@@ -407,6 +407,8 @@ class PytestRunner(TestRunner):
if mutmut.config.debug:
params = ['-vv'] + params
print('python -m pytest ', ' '.join(params))
+ if mutmut.config.pytest_extra_args:
+ params += mutmut.config.pytest_extra_args
exit_code = int(pytest.main(params, **kwargs))
if mutmut.config.debug:
print(' exit code', exit_code)
@@ -676,6 +678,7 @@ class Config:
debug: bool
paths_to_mutate: List[Path]
tests_dir: List[str] = None
+ pytest_extra_args: List[str] = None
def should_ignore_for_mutation(self, path):
if not str(path).endswith('.py'):
@@ -756,6 +759,7 @@ def load_config():
for y in s('paths_to_mutate', [])
] or guess_paths_to_mutate(),
tests_dir=s('tests_dir', []),
+ pytest_extra_args=s('pytest_extra_args', []),
)
.github/workflows/test.yaml
Outdated
| if: github.event_name == 'pull_request' | ||
| strategy: | ||
| matrix: | ||
| fork: [frontier, homestead, tangerine_whistle, spurious_dragon, byzantium, constantinople, istanbul, berlin, london, paris, shanghai, cancun, prague, osaka] |
There was a problem hiding this comment.
some forks do not have a direct tests/{fork_name} file - what should we do for them?
There was a problem hiding this comment.
This is likely going to change with @gurukamath's recent work on refactoring the tests.
There was a problem hiding this comment.
The mutation tests might take a while to run - so I'm wondering how we should run them - per PR or commit on main
If several files have changes the time to run on CI also might be unrealistic. In which case I was thinking if we should add more limitations on when to run these tests (or patch mutmut to be able to run mutation tests for specific functions (as opposed to files).
Attached a test run on the main comment ^
.github/workflows/test.yaml
Outdated
| run: pip install mutmut==3.3.0 | ||
| - name: Apply mutmut patch | ||
| run: | | ||
| git clone https://github.com/souradeep-das/mutate-execution-specs.git |
There was a problem hiding this comment.
Instead of patching, you can install a git dependency:
pip install git+https://github.com/souradeep-das/mutmut.git@<commit hash>I think that would be easier to maintain than a patch file.
On a separate note, would something like this work (and maybe we can upstream)?
diff --git a/mutmut/__main__.py b/mutmut/__main__.py
index 0fb1ad9..15b16ef 100644
--- a/mutmut/__main__.py
+++ b/mutmut/__main__.py
@@ -407,6 +407,8 @@ class PytestRunner(TestRunner):
if mutmut.config.debug:
params = ['-vv'] + params
print('python -m pytest ', ' '.join(params))
+ if mutmut.config.pytest_extra_args:
+ params += mutmut.config.pytest_extra_args
exit_code = int(pytest.main(params, **kwargs))
if mutmut.config.debug:
print(' exit code', exit_code)
@@ -676,6 +678,7 @@ class Config:
debug: bool
paths_to_mutate: List[Path]
tests_dir: List[str] = None
+ pytest_extra_args: List[str] = None
def should_ignore_for_mutation(self, path):
if not str(path).endswith('.py'):
@@ -756,6 +759,7 @@ def load_config():
for y in s('paths_to_mutate', [])
] or guess_paths_to_mutate(),
tests_dir=s('tests_dir', []),
+ pytest_extra_args=s('pytest_extra_args', []),
)
.github/workflows/test.yaml
Outdated
| if: github.event_name == 'pull_request' | ||
| strategy: | ||
| matrix: | ||
| fork: [frontier, homestead, tangerine_whistle, spurious_dragon, byzantium, constantinople, istanbul, berlin, london, paris, shanghai, cancun, prague, osaka] |
There was a problem hiding this comment.
This is likely going to change with @gurukamath's recent work on refactoring the tests.
.github/workflows/test.yaml
Outdated
| id: changed | ||
| run: | | ||
| git fetch origin ${{ github.event.pull_request.base.ref }} | ||
| CHANGED_FILES=$(git diff --name-only origin/${{ github.event.pull_request.base.ref }} ${{ github.sha }} | grep "^src/ethereum/${{ matrix.fork }}/" || true) |
There was a problem hiding this comment.
Do you need to grep, or can you just do something like git diff ... -- src/ethereum/${{ matrix.fork }}/?
.github/workflows/test.yaml
Outdated
| CHANGED_FILES=$(git diff --name-only origin/${{ github.event.pull_request.base.ref }} ${{ github.sha }} | grep "^src/ethereum/${{ matrix.fork }}/" || true) | ||
| echo "changed_files<<EOF" >> $GITHUB_OUTPUT | ||
| echo "$CHANGED_FILES" >> $GITHUB_OUTPUT | ||
| echo "EOF" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
| CHANGED_FILES=$(git diff --name-only origin/${{ github.event.pull_request.base.ref }} ${{ github.sha }} | grep "^src/ethereum/${{ matrix.fork }}/" || true) | |
| echo "changed_files<<EOF" >> $GITHUB_OUTPUT | |
| echo "$CHANGED_FILES" >> $GITHUB_OUTPUT | |
| echo "EOF" >> $GITHUB_OUTPUT | |
| CHANGED_FILES=$() | |
| echo "changed_files<<EOF" >> $GITHUB_OUTPUT | |
| ( git diff --name-only origin/${{ github.event.pull_request.base.ref }} ${{ github.sha }} | grep "^src/ethereum/${{ matrix.fork }}/" || true ) >> $GITHUB_OUTPUT | |
| echo "EOF" >> $GITHUB_OUTPUT |
Can avoid storing the list in a temporary variable.
.github/workflows/test.yaml
Outdated
| echo '[tool.mutmut]' > pyproject.toml | ||
| FILES=$(echo "${{ steps.changed.outputs.changed_files }}" | awk '{printf "\"%s\", ", $0}' | sed 's/, $//') | ||
| echo "paths_to_mutate = [ $FILES ]" >> pyproject.toml | ||
| echo "tests_dir = [\"tests/${{ matrix.fork }}/\"]" >> pyproject.toml | ||
| echo "debug = true" >> pyproject.toml |
There was a problem hiding this comment.
Hm, this is... unusual.
So first, this might be easier to implement with shell: python instead of bash. tomllib is in the standard library.
Second, we shouldn't need to modify our project configuration at all to get this to work. I think it would make sense to try and get a patch into mutmut that allows passing pytest arguments via the command line (unlike my patch above) or maybe via an environment variable. We can't be the only project with stupidly long running tests.
There was a problem hiding this comment.
On a separate note, would something like this work (and maybe we can upstream)?
Is the patch you mentioned above, for any specific cases that we have in mind? or is it just intended to supply in options like test_dir?
I updated mutmut to take in options through the cli - and its on a PR upstream
There was a problem hiding this comment.
also looks like mutmut is not particularly leaning towards enabling the cli options
There was a problem hiding this comment.
| run: tox -e optimized | ||
|
|
||
| mutation-test: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
| runs-on: ubuntu-latest | |
| runs-on: [self-hosted-ghr, size-xl-x64] |
This is the magic sauce that runs the job on a beefier machine.
.github/workflows/test.yaml
Outdated
| if: github.event_name == 'pull_request' | ||
| strategy: | ||
| matrix: | ||
| fork: [frontier, homestead, tangerine_whistle, spurious_dragon, byzantium, constantinople, istanbul, berlin, london, paris, shanghai, cancun, prague, osaka] |
There was a problem hiding this comment.
If you generate the matrix from the output of a previous job (i.e. ${{ needs.changed-forks.outputs.changed-forks }} where changed-forks is a preceding job that stores the changed forks), you can cut down own the number of runs plus use ethereum_spec_tools.forks:Hardfork to dynamically generate the list instead of having to hard code it.
There was a problem hiding this comment.
thanks - that sounds great!
What was wrong?
Related to Issue #1362 and #1145
How was it fixed?
Test run on fork: https://github.com/souradeep-das/execution-specs/actions/runs/17051785519
Note - needs #1361 to be merged
Cute Animal Picture