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

Feat: mutation testing integrated flows #3

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ASuciuX
Copy link
Contributor

@ASuciuX ASuciuX commented Dec 11, 2023

WIP in order to merge

  • add CI workflows
  • fill this PR info
  • update docs
  • upload initial sync mutants output
    • upload most of the sync without a few which are having problems on mac os

Description

Add 2 main new workflows for the mutation testing: filter-pr and logger. They interact with the stacks blockchain repo and keep track and filter the mutations for the PR and push actions there. They are intended to be run on master, develop and next.

Logger uses 2 smaller workflows:

  1. upload-initial-cache if no cache is found on stacks repository. It will upload using the local output for the specific branch.
  2. update-cache if a cache exists. It will take the old cache, update it with the output from the cargo mutants, which are run on the functions that are highlighted in the git diff.

It also includes the scripts that are run and the last synced outputs for the master, develop and next branches of the stacks blockchain
repository.


  1. Motivation for change
    To have mutations testing for better coverage, keeping track of untested functions and testing them as much as possible afterwards.
  2. What was changed
    I've added the workflows.
  3. How does this impact application developers
    They will need to have tests for the functions they are committing in the PR. If there are missing/timeout/unviable tests the CI workflow will crash. In the case that a specific function shouldn't be tested, it can be highlighted so by specifying in its header #[mutants::skip] or other attributes containing mutants::skip (e.g. #[cfg_attr(test, mutants::skip)]).
  4. Link to relevant issues and documentation
    Mutation Testing ( splitted into 2 PRs ) stacks-core#4089
    https://github.com/sourcefrog/cargo-mutants
    https://mutants.rs/

Type of Change

  • New feature

Does this introduce a breaking change?

No

Are documentation updates required?

No. I've included readme files as well.

needs initial output pushed into this branch before merging
run: |
mv git.diff ./actions-repo/mutation-testing/shell-scripts/

- uses: Swatinem/rust-cache@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we have been pinning external actions by hash to avoid possible breakage or supply chain attacks

id: git_checkout_stacks_core
uses: actions/checkout@v3
with:
fetch-depth: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

fetch-depth: 0 fetches entire history. Can we optimize by only fetching the branches (or commits) we are comparing?

## search in that specific folder on all 4 files
## if it is matchy, remove it from that file
## based on the file it was taken from, append it to the same file in the STABLE folder

Copy link
Contributor

Choose a reason for hiding this comment

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

It's considered good practice to add set -euo pipefail to all bash scripts. This will cause cause failures on certain errors, rather continuing to run and producing bad results

# the append-match-package.sh
## goes through each line in the output and based on the package ( first element before /)
### verifies the line with the other lines in that specific folder
#### in our case folder_name == package_name
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the extra #s?


# removes everything except .txt files

#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be the first line in the file

#!/bin/bash

# moves to mutation-testing folder
cd ../packages-output
Copy link
Contributor

Choose a reason for hiding this comment

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

Shellcheck warning:

cd ../packages-output
^-------------------^ SC2164 (warning): Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

@@ -0,0 +1,41 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

This script needs some comments

impl_block="${impl_block%%\ {*}"
impl_block="<impl$impl_block>" # add 'impl' word, '<' and '>' characters
impl_block=$(echo "$impl_block" | sed -E 's/(^[^ ]*impl)[^ ]*/\1/') # remove everything after 'impl', until the next space
impl_block=$(echo "$impl_block" | sed 's/\(<[^<]*\)[^ ]* /\1 /') # remove everything starting with '<' and ending at the next space, from the second word
Copy link
Contributor

Choose a reason for hiding this comment

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

Shellcheck warning:

    impl_block=$(echo "$impl_block" | sed 's/\(<[^<]*\)[^ ]* /\1 /') # remove everything starting with '<' and ending at the next space, from the second word
                 ^-- SC2001 (style): See if you can use ${variable//search/replace} instead.

shell: bash
run: |
if [ -f ./packages-output/last_commit_hash.txt ]; then
cargo mutants --no-shuffle -j 2 -vV --in-diff ./scripts/git.diff --output temp/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the diff in the ./scripts directory?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the first half of this file looks the same as filter-pr. Why not use that?


- name: Remove deleted file lines from git.diff file
shell: bash
run: ./remove-deleted-file-lines.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Shouldn't cargo mutants be able to read a diff and know test deleted functions?

@wileyj
Copy link
Collaborator

wileyj commented Jan 25, 2024

I'm not a fan of keeping the raw text output in this repo in that initial-output folder - are there alternatives you've explored for keeping this data, and more importantly keeping this data up to date?

for example, some process should be periodically updating this output, correct? would a non-versioned storage location work better or is the versioning imperative for this data?

@ASuciuX
Copy link
Contributor Author

ASuciuX commented Jan 25, 2024

Yes, I'll save it somewhere else and we could remove it from here afterwards. Also, as a non stable output i was thinking of the cache as it is used in the actions workflow process, but not sure what's the best stable place to keep it. ( updating a github repo, something automated )

@wileyj
Copy link
Collaborator

wileyj commented Jan 25, 2024 via email

@ASuciuX ASuciuX marked this pull request as draft February 14, 2024 22:18
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.

None yet

3 participants