Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Add a GetExpand trait for initializing Expand objects in our tasks #3553

Merged
merged 36 commits into from
Oct 19, 2023

Conversation

kananb
Copy link
Contributor

@kananb kananb commented Oct 5, 2023

Summary of the Pull Request

Implements a slightly more standardized way for initializing Expand objects by adding a GetExpand trait that defines a similarly named function. Each config then implements it to generate the Expand object.

Property-based unit tests are also added to ensure that all of the variables that are expected to be available for expansion, in fact, are.

A few things that could use further consideration:

  1. Expand object parameters sometimes come from the function's arguments, not always from just the config fields.
    1. target_exe in the generic coverage task.
    2. generated_inputs and input_corpus in the generator task.
    3. runtime_dir and crashes in the supervisor task.
    4. generated_inputs in the merge task.
  2. Sometimes a value is calculated asynchronously and then passed to the Expand object. I'm not sure how to accommodate this from the trait implementation when the value is calculated using only config fields.
    1. target_exe in the analysis task.
    2. target_exe in the dotnet coverage task.
    3. target_exe in the supervisor task.
    4. target_exe in the merge task.
  3. In a few places, a function argument is passed to the Expand object that overrides the value that was passed from the config.
    1. target_exe and reports_dir in the analysis task.
    2. target_exe in the dotnet coverage task.
    3. input_corpus, reports_dir, and crashdumps in the supervisor task.
  4. I'm still very new to Rust, so I might probably have done some silly things without realizing.

PR Checklist

  • Applies to work item: Some parameters are not available for expansion. #3024
  • CLA signed. If not, go over here and sign the CLI.
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Info on Pull Request

What does this include?

Validation Steps Performed

How does someone test & validate?

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

Merging #3553 (d859cbb) into main (90be543) will increase coverage by 0.95%.
The diff coverage is 81.30%.

@@            Coverage Diff             @@
##             main    #3553      +/-   ##
==========================================
+ Coverage   39.01%   39.96%   +0.95%     
==========================================
  Files         302      303       +1     
  Lines       36963    37635     +672     
  Branches        0     1721    +1721     
==========================================
+ Hits        14421    15042     +621     
+ Misses      22542    22274     -268     
- Partials        0      319     +319     
Files Coverage Δ
src/agent/onefuzz-task/src/local/template.rs 77.27% <100.00%> (-1.46%) ⬇️
src/agent/onefuzz-task/src/tasks/config.rs 36.43% <100.00%> (+28.36%) ⬆️
...c/agent/onefuzz-task/src/tasks/coverage/generic.rs 88.56% <100.00%> (+0.68%) ⬆️
src/agent/onefuzz-task/src/local/common.rs 10.95% <0.00%> (+0.47%) ⬆️
src/agent/onefuzz-task/src/config_test_utils.rs 99.46% <99.46%> (ø)
...nt/onefuzz-task/src/tasks/report/dotnet/generic.rs 28.03% <96.77%> (+28.03%) ⬆️
src/agent/onefuzz-task/src/tasks/merge/generic.rs 35.91% <94.44%> (+35.91%) ⬆️
...c/agent/onefuzz-task/src/tasks/analysis/generic.rs 34.76% <95.29%> (+34.76%) ⬆️
...rc/agent/onefuzz-task/src/tasks/coverage/dotnet.rs 8.69% <87.50%> (+8.69%) ⬆️
src/agent/onefuzz-task/src/tasks/fuzz/generator.rs 21.84% <44.44%> (+21.84%) ⬆️
... and 1 more

... and 52 files with indirect coverage changes

@kananb kananb marked this pull request as ready for review October 9, 2023 17:39
@kananb kananb merged commit 6668fa2 into main Oct 19, 2023
24 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants