Skip to content

Conversation

guoyr
Copy link
Contributor

@guoyr guoyr commented Aug 11, 2022

No description provided.

@guoyr guoyr requested review from a team, zamj and MikhailShchatko and removed request for a team August 11, 2022 14:56
let mut params = self
.config_extraction_service
.task_def_to_resmoke_params(task_def, false, None)?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will remove

Copy link
Collaborator

@MikhailShchatko MikhailShchatko left a comment

Choose a reason for hiding this comment

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

Looks great % some comments

}
} else {
remove_gen_suffix(&task.name)
generated_task_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add some tests for these updates?

} else {
None
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

And some tests for this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this function, my thought was to have it be internal to this module. Since we don't need to mock out functions as much in this project (and rely instead more heavily on traits), the 2 tests added for find_suite_name should catch any visible changes to this function. Please feel free to push back

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine with me

pub const GENERATE_RESMOKE_TASKS: &str = "generate resmoke tasks";

// Functions for running tests.
pub const RUN_RESMOKE_TASK: &str = "run tests";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems confusing. Maybe rename it to RUN_RESMOKE_TESTS or just RUN_TESTS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to RUN_TEST_TESTS. Adding some comments for posterity.

I'm a bit ambivalent about the naming. Conceptually, I think this function is both parallel to _gen tasks and downstream from them by being the generated task.

For the purposes of task generation logic in this codebase, including burn_in, we treat existing instances of this function as a part of the input for task generation, which means it's treated as an equal to the _gen task definitions. I therefore named it similar to generate_resmoke_tasks to avoid potential confusion of handling "generate_resmoke_tasks" and "run_tests" the same way.

@guoyr guoyr requested a review from MikhailShchatko August 12, 2022 19:42
Copy link
Contributor

@zamj zamj left a comment

Choose a reason for hiding this comment

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

LGTM % mod my comment

} else {
None
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine with me

#[test]
fn test_find_suite_name_should_use_task_name_for_non_generated_task_if_no_var() {
let evg_task = EvgTask {
name: "my_task_gen".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] I think this should be my_task as it's not a gen task

@guoyr
Copy link
Contributor Author

guoyr commented Aug 15, 2022

evergreen merge

@ghost ghost merged commit 114aead into master Aug 15, 2022
@ghost ghost deleted the robert.guo/DAG-1963 branch August 15, 2022 03:45
This pull request was closed.
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