search

fhir-crucible / testscript-engine

Ruby FHIR TestScript execution engine
Apache License 2.0
5 stars 2 forks source link

add_autocreate_delete #8

Closed jhlee-mitre closed 2 years ago

jhlee-mitre commented 2 years ago 
•   general_test_script.json -> set autocreate and autodelete -> true for testing
•   Added autocreate under 'load_fixture'. Need to discuss where the code be placed.
•   In case of autocreate = true and create exists in setup section in TS, give warning.
•   Added autodelete under 'postprocess'.Need to discuss whether we need a separate method for post processing.
ms-k1ngk0ng commented 2 years ago

General comment, it'd be helpful for me as the reviewer if you can get a little more specific with your commit messages. 'Update [file_name]' doesn't tell me a whole lot of information about what to look for. This is something I also need to get better at.

Jaehoon: I found it is interesting that I can edit other's comment. Good suggestion and I will put specifics in commit messages in the future.

jhlee-mitre commented 2 years ago

I found a problem of autocreation while testing Abi's TestScripts. Let's hold this PR until I figure it out.

jhlee-mitre commented 2 years ago

Added simple unit test to see by autocreate / autodelete = T or F, whether the method runs or not. (The way captured the result is to see if there's error from http request or not) Sort-of weird but I'm not sure how to unit test other way.

ms-k1ngk0ng commented 2 years ago

This PR is getting too massive. I think we have a couple options: 1) Break it down into more manageable bites and deal with it piece-by-piece. Pros are that this would make things much more organized and cons is the added lift of sifting through all these changes and making PRs for them -- though that might be a good task to get situated with all the changes. 2) Merge in as-is and then immediately comb through the code to fix anything that breaks. Pros are this is way easier than divvying this up piece-by-piece and we can deal with everything in one fell swoop and cons are that this is more disorganized and may potentially break things on main until we get fixes merged in. I'm okay with either @jhlee-mitre but I think we need to deal with this PR in the next couple of days and just take it off both of our plates :)

jhlee-mitre commented 2 years ago

This PR is getting too massive. I think we have a couple options:

  1. Break it down into more manageable bites and deal with it piece-by-piece. Pros are that this would make things much more organized and cons is the added lift of sifting through all these changes and making PRs for them -- though that might be a good task to get situated with all the changes.
  2. Merge in as-is and then immediately comb through the code to fix anything that breaks. Pros are this is way easier than divvying this up piece-by-piece and we can deal with everything in one fell swoop and cons are that this is more disorganized and may potentially break things on main until we get fixes merged in. I'm okay with either @jhlee-mitre but I think we need to deal with this PR in the next couple of days and just take it off both of our plates :)

I would vote to the option 1 as I started being scared of merging the outdated codes even if we comb it immediately after. I would like to start over, rewriting codes by good size of pieces then PR one by one.

If we close this PR, can we still keep it archived? Or will it be removed?

jhlee-mitre commented 2 years ago

Close and will create a new branch for autocreate/autodelete from recently merged main.