Closed barrettk closed 2 years ago
Still need to add some safeguards and informative warnings. Informative warnings will likely take some time based on how the function is currently set up
@seth127 @kyleam
Here is what the current functionality looks like:
parse_github_issues(org = ORG, repo = REPO, mile = MILESTONES, domain = DOMAIN) %>%
assign_test_ids(test_type = "test_that")
which returns:
> parse_github_issues(org = ORG, repo = REPO, mile = MILESTONES, domain = DOMAIN) %>%
+ assign_test_ids(test_type = "test_that")
Joining, by = "TestId"
Warning: The following tests were not found in github milestones.
The corresponding Test Id's have still been added to the test files.
TestName TestId
1 submit_model(.dry_run=T) with bbi_nonmem_model object parses correctly TST-FOO-108
2 combine_directory_path() builds the expected path with NULL .directory TST-FOO-055
3 create_run_log_object() errors if ABS_MOD_PATH has missing values TST-FOO-013
4 combine_list_objects() correctly fails if .func_args isn't named TST-FOO-126
5 submit_models(.dry_run=T) with character and numeric input yaml TST-FOO-113
6 create_run_log_object() errors if ABS_MOD_PATH is not character TST-FOO-011
7 create_model_object() fails with non-valid model file extension TST-FOO-003
8 compare read_model() and new_model() objects with numeric input TST-FOO-097
9 combine_directory_path() builds fake .path in real .directory TST-FOO-056
10 submit_model(.dry_run=T) with numeric input parses correctly TST-FOO-109
11 get_path_from_object.bbi_run_log_df() builds the right paths TST-FOO-037
12 create_run_log_object() errors if ABS_MOD_PATH is not unique TST-FOO-012
13 combine_directory_path() builds the expected path .directory TST-FOO-054
14 parse_args_list() correctly fails if .func_args isn't named TST-FOO-123
15 find_model_file_path returns mod path when only path found TST-FOO-049
16 submit_models(.dry_run=T) with character mixed extensions TST-FOO-114
17 submit_model(.dry_run=T) with .ctl input parses correctly TST-FOO-105
18 as_model() returns the correct type from a process object TST-FOO-095
19 find_model_file_path returns ctl path when no path found TST-FOO-048
20 submit_model(.dry_run=T) returns correct command string TST-FOO-104
21 as_model() returns the correct type from a model object TST-FOO-094
22 get_path_from_object.character() builds the right path TST-FOO-034
23 add_decisions() and replace_decisions() work correctly TST-FOO-061
24 submit_models(.dry_run=T) with list input, 2 arg sets TST-FOO-111
25 create_model_object() fails with non-valid model type TST-FOO-002
26 get_path_from_object.default() builds the right path TST-FOO-033
27 get_path_from_object.character() .check_exists works TST-FOO-036
28 combine_directory_path() errors with fake .directory TST-FOO-057
29 add_bbi_args() and replace_bbi_args() work correctly TST-FOO-063
30 get_path_from_object.character() fails with vector TST-FOO-035
31 create_summary_object() errors if keys are missing TST-FOO-006
32 create_run_log_object() errors if keys are missing TST-FOO-010
33 create_process_object() errors if keys are missing TST-FOO-008
34 submit_models(.dry_run=T) with list input simple TST-FOO-110
35 get_model_ancestry.character() fails with vector TST-FOO-029
36 create_model_object() errors if keys are missing TST-FOO-004
37 submit_models(.dry_run=T) errors with bad input TST-FOO-112
38 save_model_yaml() saves to correct default path TST-FOO-089
39 create_summary_object() correctly assigns class TST-FOO-005
40 create_run_log_object() correctly assigns class TST-FOO-009
41 create_process_object() correctly assigns class TST-FOO-007
42 combine_list_objects() merges lists as expected TST-FOO-124
43 new_model() .based_on arg errors on fake model TST-FOO-088
44 combine_list_objects() merges with append=TRUE TST-FOO-125
45 save_model_yaml() saves to user supplied path TST-FOO-090
46 get_path_from_object() errors on vector field TST-FOO-039
47 get_path_from_object() errors on missing keys TST-FOO-038
48 find_model_file_path returns correct ctl path TST-FOO-046
49 create_model_object() correctly assigns class TST-FOO-001
50 config_log() works correctly with nested dirs TST-FOO-101
51 add_config() works correctly with nested dirs TST-FOO-102
52 save_model_yaml() doesn't save an empty list TST-FOO-092
53 compare read_model() and new_model() objects TST-FOO-085
54 add_tags() and replace_tags() work correctly TST-FOO-060
55 parse_args_list() merges lists as expected TST-FOO-121
56 parse_args_list() handles NULL as expected TST-FOO-122
57 get_path_from_object() errors on fake path TST-FOO-040
58 get_based_on.character() fails with vector TST-FOO-022
59 build_bbi_param_list happy path single set TST-FOO-117
60 build_bbi_param_list dies with a non model TST-FOO-120
61 modify_model_field() de-duplication works TST-FOO-059
62 as_model() errors with non-existent model TST-FOO-096
63 save_model_yaml() saves tags as an array TST-FOO-093
64 save_model_yaml() deletes the right keys TST-FOO-091
65 new_model() fails with invalid yaml path TST-FOO-084
66 build_bbi_param_list happy path two sets TST-FOO-118
67 yaml with no model path will return ctl TST-FOO-083
68 run_log() works with both yaml and yml TST-FOO-099
69 get_output_dir() builds the right path TST-FOO-042
70 get_model_path() builds the right path TST-FOO-041
71 run_log fails after messing with YAML TST-FOO-103
72 replace_description() works correctly TST-FOO-062
73 get_yaml_path() builds the right path TST-FOO-043
74 find_model_file_path prefers ctl path TST-FOO-047
75 copy_from_model creates accurate copy TST-FOO-014
76 check_required_keys() works correctly TST-FOO-127
77 read_model() returns expected object TST-FOO-078
78 modify_model_field() works correctly TST-FOO-058
79 build_bbi_param_list .bbi_args works TST-FOO-119
80 strict_mode_error() works correctly TST-FOO-128
81 yaml with bad model path will fail TST-FOO-082
82 check_nonmem_args parses correctly TST-FOO-115
83 yaml with no model type will fail TST-FOO-081
84 is_valid_nonmem_extension() works TST-FOO-044
85 format_cmd_args parses correctly TST-FOO-116
86 copy_from_model bbi_nonmem_model TST-FOO-016
87 suppressSpecificWarning() works TST-FOO-129
88 is_valid_yaml_extension() works TST-FOO-045
89 copy_from_model options work TST-FOO-015
90 add_tags etc. can be chained TST-FOO-064
91 copy_from_model numeric TST-FOO-019
Warning: The following github issues did not have a matching test.
The test files containing these tests have been filtered out.
missed_tests
1 basic workflow is correct using rbabylon.model_directory = 'model-examples'
2 basic workflow is correct using rbabylon.model_directory = 'model-examples' change_midstream
3 summary call is the same using rbabylon.model_directory = 'model-examples' .
4 summary call is the same using rbabylon.model_directory = 'model-examples' ..
5 summary call is the same using rbabylon.model_directory = 'model-examples' model-examples
6 summary call is the same using rbabylon.model_directory = 'model-examples' . change_midstream
7 summary call is the same using rbabylon.model_directory = 'model-examples' .. change_midstream
8 summary call is the same using rbabylon.model_directory = 'model-examples' model-examples change_midstream
9 basic workflow is correct from different working directories .
10 basic workflow is correct from different working directories ..
11 basic workflow is correct from different working directories model-examples
12 basic workflow is correct from different working directories . change_midstream
13 basic workflow is correct from different working directories .. change_midstream
14 basic workflow is correct from different working directories model-examples change_midstream
15 Summary call is the same after changing directories .
16 Summary call is the same after changing directories ..
17 Summary call is the same after changing directories model-examples
18 Summary call is the same after changing directories . change_midstream
19 Summary call is the same after changing directories .. change_midstream
20 Summary call is the same after changing directories model-examples change_midstream
21 find_config_file_path() parses correctly 1
22 find_config_file_path() parses correctly 2
23 find_config_file_path() parses correctly 3
24 find_config_file_path() errors correctly 1
25 find_config_file_path() errors correctly 2
26 JUL-ROC-001
27 BBR-BBR-005
28 BBR-BBR-006
29 BBR-BBR-007
# A tibble: 4 × 5
StoryId StoryName StoryDescription ProductRisk TestIds
<chr> <chr> <chr> <chr> <chr>
1 mrgvalidatetestreference-1 based_on tag correctly stores relative paths "User should be able to use the `based_on` field to unambiguously refer to … low TST-FO…
2 mrgvalidatetestreference-2 support both yml and yaml extensions "A user can use either a .yaml or .yml file for the model file.\r\n\r\n**Re… low TST-FO…
3 mrgvalidatetestreference-4 Copying models in typical workflow "A user should be able to call `copy_model_from()` without worrying about a… low TST-FO…
4 mrgvalidatetestreference-5 Model object should stay in sync with YAML file "The model YAML must serve as a \"source of truth\" for the data that it st… low TST-FO…
Accompanied by stories_to_yaml
:
parse_github_issues(org = ORG, repo = REPO, mile = MILESTONES, domain = DOMAIN) %>%
assign_test_ids(test_type = "test_that") %>%
stories_to_yaml(file = file.path(tempdir(), "temp.yaml")) %>%
file.edit()
The first dataframe of warnings is not as big of a deal. For those instances, the new test IDs are generated and added to the test files. However, since we're missing a storyId and the other relevant parameters, those tests wont be added to the yaml file.
The second dataframe is the larger issue. In those cases the tests outlined in the milestones weren't found in the testing directory. In this example we have one "fake" milestone, where those tests were never written. However we also have tests that were written in such a way, where we cant pick them up (see issue https://github.com/metrumresearchgroup/mrgvalprep/issues/33). Those cases must be handled manually.
I think the first list of warnings could be handled better, but would like to get some advice there.
Suggestions: 1) I think we should add issues (manually) under milestone v0.6.0 for all tests missing:
Warning: The following tests were not found in github milestones.
The corresponding Test Id's have still been added to the test files:
TestName TestId
1 submit_model(.dry_run=T) with bbi_nonmem_model object parses correctly TST-FOO-108
2 combine_directory_path() builds the expected path with NULL .directory TST-FOO-055
3 create_run_log_object() errors if ABS_MOD_PATH has missing values TST-FOO-013
4 combine_list_objects() correctly fails if .func_args isn't named TST-FOO-126
5 submit_models(.dry_run=T) with character and numeric input yaml TST-FOO-113
6 create_run_log_object() errors if ABS_MOD_PATH is not character TST-FOO-011
7 create_model_object() fails with non-valid model file extension TST-FOO-003
8 compare read_model() and new_model() objects with numeric input TST-FOO-097
9 combine_directory_path() builds fake .path in real .directory TST-FOO-056
10 submit_model(.dry_run=T) with numeric input parses correctly TST-FOO-109
11 get_path_from_object.bbi_run_log_df() builds the right paths TST-FOO-037
12 create_run_log_object() errors if ABS_MOD_PATH is not unique TST-FOO-012
13 combine_directory_path() builds the expected path .directory TST-FOO-054
14 parse_args_list() correctly fails if .func_args isn't named TST-FOO-123
15 find_model_file_path returns mod path when only path found TST-FOO-049
16 submit_models(.dry_run=T) with character mixed extensions TST-FOO-114
17 submit_model(.dry_run=T) with .ctl input parses correctly TST-FOO-105
18 as_model() returns the correct type from a process object TST-FOO-095
19 find_model_file_path returns ctl path when no path found TST-FOO-048
20 submit_model(.dry_run=T) returns correct command string TST-FOO-104
21 as_model() returns the correct type from a model object TST-FOO-094
22 get_path_from_object.character() builds the right path TST-FOO-034
23 add_decisions() and replace_decisions() work correctly TST-FOO-061
24 submit_models(.dry_run=T) with list input, 2 arg sets TST-FOO-111
25 create_model_object() fails with non-valid model type TST-FOO-002
26 get_path_from_object.default() builds the right path TST-FOO-033
27 get_path_from_object.character() .check_exists works TST-FOO-036
28 combine_directory_path() errors with fake .directory TST-FOO-057
29 add_bbi_args() and replace_bbi_args() work correctly TST-FOO-063
30 get_path_from_object.character() fails with vector TST-FOO-035
31 create_summary_object() errors if keys are missing TST-FOO-006
32 create_run_log_object() errors if keys are missing TST-FOO-010
33 create_process_object() errors if keys are missing TST-FOO-008
34 submit_models(.dry_run=T) with list input simple TST-FOO-110
35 get_model_ancestry.character() fails with vector TST-FOO-029
36 create_model_object() errors if keys are missing TST-FOO-004
37 submit_models(.dry_run=T) errors with bad input TST-FOO-112
38 save_model_yaml() saves to correct default path TST-FOO-089
39 create_summary_object() correctly assigns class TST-FOO-005
40 create_run_log_object() correctly assigns class TST-FOO-009
41 create_process_object() correctly assigns class TST-FOO-007
42 combine_list_objects() merges lists as expected TST-FOO-124
43 new_model() .based_on arg errors on fake model TST-FOO-088
44 combine_list_objects() merges with append=TRUE TST-FOO-125
45 save_model_yaml() saves to user supplied path TST-FOO-090
46 get_path_from_object() errors on vector field TST-FOO-039
47 get_path_from_object() errors on missing keys TST-FOO-038
48 find_model_file_path returns correct ctl path TST-FOO-046
49 create_model_object() correctly assigns class TST-FOO-001
50 config_log() works correctly with nested dirs TST-FOO-101
51 add_config() works correctly with nested dirs TST-FOO-102
52 save_model_yaml() doesn't save an empty list TST-FOO-092
53 compare read_model() and new_model() objects TST-FOO-085
54 add_tags() and replace_tags() work correctly TST-FOO-060
55 parse_args_list() merges lists as expected TST-FOO-121
56 parse_args_list() handles NULL as expected TST-FOO-122
57 get_path_from_object() errors on fake path TST-FOO-040
58 get_based_on.character() fails with vector TST-FOO-022
59 build_bbi_param_list happy path single set TST-FOO-117
60 build_bbi_param_list dies with a non model TST-FOO-120
61 modify_model_field() de-duplication works TST-FOO-059
62 as_model() errors with non-existent model TST-FOO-096
63 save_model_yaml() saves tags as an array TST-FOO-093
64 save_model_yaml() deletes the right keys TST-FOO-091
65 new_model() fails with invalid yaml path TST-FOO-084
66 build_bbi_param_list happy path two sets TST-FOO-118
67 yaml with no model path will return ctl TST-FOO-083
68 run_log() works with both yaml and yml TST-FOO-099
69 get_output_dir() builds the right path TST-FOO-042
70 get_model_path() builds the right path TST-FOO-041
71 run_log fails after messing with YAML TST-FOO-103
72 replace_description() works correctly TST-FOO-062
73 get_yaml_path() builds the right path TST-FOO-043
74 find_model_file_path prefers ctl path TST-FOO-047
75 copy_from_model creates accurate copy TST-FOO-014
76 check_required_keys() works correctly TST-FOO-127
77 read_model() returns expected object TST-FOO-078
78 modify_model_field() works correctly TST-FOO-058
79 build_bbi_param_list .bbi_args works TST-FOO-119
80 strict_mode_error() works correctly TST-FOO-128
81 yaml with bad model path will fail TST-FOO-082
82 check_nonmem_args parses correctly TST-FOO-115
83 yaml with no model type will fail TST-FOO-081
84 is_valid_nonmem_extension() works TST-FOO-044
85 format_cmd_args parses correctly TST-FOO-116
86 copy_from_model bbi_nonmem_model TST-FOO-016
87 suppressSpecificWarning() works TST-FOO-129
88 is_valid_yaml_extension() works TST-FOO-045
89 copy_from_model options work TST-FOO-015
90 add_tags etc. can be chained TST-FOO-064
91 copy_from_model numeric TST-FOO-019
2) I think we should remove this issue from that milestone, and add it to the fake milestone (or a new one) that's specifically included to capture warnings.
If we do both of these things, we shouldnt get any warnings anymore. However, the function would still fail to pick up the tests included in that issue, as they dont follow the normal convention, and utilize glue to create the function name.
Had to change regex for the yaml creation function only. The new way ([\\s,;]+
) would have produced the following:
The old way (, *"
) produces what we want:
Note that this regex change was only made to stories_to_yaml()
. The new way makes more sense for the other functions, but stories_to_yaml()
only works with the old regex. I have confirmed this wont be an issue regardless of where the stories dataframe comes from.
Unfortunately found another bug while writing tests:
I accounted for this type of issue earlier on in the function (by first sorting by number of characters), but will need a new solution for updating the actual test files (previous solution wont work since one string is still a subset of another). Unfortunately it looks like Ill have to break up part of that function and perform the updates in an itemized fasion.
The new function and relevant tests are now ready for review. Didnt write tests for the helper functions I made, but can do so if we think it's necessary
FYI @kylebaron The function is ready for testing if you feel like giving it a try/testing out the functionality
New results:
source("/data/Projects/package_dev/mrgvalprep/tests/testthat/setup.R", echo=FALSE)
options(TEST_DIR = system.file("fake-tests", package = "mrgvalprep"))
options(TEST_DIR_TESTING = TRUE)
MILESTONES <- c("v0.6.0", "v0.6.1")
parse_github_issues(org = ORG, repo = REPO, mile = MILESTONES, domain = DOMAIN) %>%
assign_test_ids(test_type = "test_that") %>%
stories_to_yaml(file = file.path(tempdir(), "temp.yaml")) %>%
file.edit()
Warning: The following github issues did not have a matching test.
Consider modifying the milestone/issue to ensure they are recognized.
$StoryId
[1] "mrgvalidatetestreference-3"
$StoryName
[1] "Check for babylon.yaml in model directory"
$StoryDescription
[1] "The user should only need to create one babylon.yaml configuration file, and it should be found by all `bbi` calls that need it automatically (if it is in the model directory). \r\n\r\n**Requirements:**\r\n\r\n* All calls that need `babylon.yaml` will look for it (and error if it can't be found) before calling out to `bbi`\r\n* When user does not explicitly specify a path to a `babylon.yaml` it will be looked for in `getOption(\"rbabylon.model_directory\")`\r\n* If the user wants to use a different configuration file for some models, they can pass the path to that into the `submit_model()` calls explicitly."
$ProductRisk
[1] "low"
$TestIds
$TestIds[[1]]
[1] "basic workflow is correct using rbabylon.model_directory = 'model-examples'"
[2] "basic workflow is correct using rbabylon.model_directory = 'model-examples' change_midstream"
[3] "summary call is the same using rbabylon.model_directory = 'model-examples' ."
[4] "summary call is the same using rbabylon.model_directory = 'model-examples' .."
[5] "summary call is the same using rbabylon.model_directory = 'model-examples' model-examples"
[6] "summary call is the same using rbabylon.model_directory = 'model-examples' . change_midstream"
[7] "summary call is the same using rbabylon.model_directory = 'model-examples' .. change_midstream"
[8] "summary call is the same using rbabylon.model_directory = 'model-examples' model-examples change_midstream"
[9] "basic workflow is correct from different working directories ."
[10] "basic workflow is correct from different working directories .."
[11] "basic workflow is correct from different working directories model-examples"
[12] "basic workflow is correct from different working directories . change_midstream"
[13] "basic workflow is correct from different working directories .. change_midstream"
[14] "basic workflow is correct from different working directories model-examples change_midstream"
[15] "Summary call is the same after changing directories ."
[16] "Summary call is the same after changing directories .."
[17] "Summary call is the same after changing directories model-examples"
[18] "Summary call is the same after changing directories . change_midstream"
[19] "Summary call is the same after changing directories .. change_midstream"
[20] "Summary call is the same after changing directories model-examples change_midstream"
[21] "find_config_file_path() parses correctly 1"
[22] "find_config_file_path() parses correctly 2"
[23] "find_config_file_path() parses correctly 3"
[24] "find_config_file_path() errors correctly 1"
[25] "find_config_file_path() errors correctly 2"
@seth127 Not necessarily asking you to review PR too, just wanted to tag you in it
@kyleam I refactored a lot, and I think the new way does make a lot more sense. We still have the regex issue, but here is the new method for calling the functions (only need to set one option for testing now):
devtools::load_all()
source("/data/Projects/package_dev/mrgvalprep/tests/testthat/setup.R", echo=FALSE)
options(TEST_DIR_TESTING = TRUE)
MILESTONES <- c("v0.6.0", "v0.6.1")
stories_df <- parse_github_issues(org = ORG, repo = REPO, mile = MILESTONES, domain = DOMAIN)
test_ids <- assign_test_ids(test_path = system.file("fake-tests", package = "mrgvalprep"))
milestone_to_test_id(stories_df = stories_df, test_ids = test_ids)
# With piping:
milestone_to_test_id(stories_df = stories_df, test_ids = test_ids) %>%
stories_to_yaml(file = file.path(tempdir(), "temp.yaml")) %>%
file.edit()
The direction looks good to me. I'll do another pass, including test files and other code outside of R/assign-test-ids.R
, but feel free to push more updates.
check
flags some things, including test failures. (The "non-standard top-level files" note can be ignored; that's me littering in my local repo.)
@kyleam Just updated the tests. They should work now.
Edit: apparently not
Unresolved comments that I spot from previous rounds
[x] unused test_lines
https://github.com/metrumresearchgroup/mrgvalprep/pull/36#discussion_r843287106
[x] unnecessary new dependency on assertthat
https://github.com/metrumresearchgroup/mrgvalprep/pull/36#discussion_r843287570
[x] question about comma splitting of milestone_tests_ref
https://github.com/metrumresearchgroup/mrgvalprep/pull/36#discussion_r843287811
[x] ID injection will still fail if test name has $
https://github.com/metrumresearchgroup/mrgvalprep/pull/36#discussion_r843286812
There's also using join
rather than for-loop in milestone_to_test_id
(comment), but we can leave this be. Let me look over the loop logic in more detail, and make sure I don't spot any issues. (edit: stepped through it, and didn't spot any obvious problems in the logic)
The new functionality will be executed as follows:
Users can modify the following options for testing this function:
getOption("TEST_DIR")
will be automatically set to the current package testing location upon loading this package: