Remove old endpoints, add tests #523

r-ash · 2024-09-06T12:53:53Z

This PR will

Remove old unused endpoints
Write tests for new endpoints

Note I left the time series endpoints as they are for now. When you tidy that up perhaps we should remove those? Though not too much harm leaving them in

r-ash · 2024-09-09T11:12:44Z

R/endpoints.R

-    ## Update some labels to match what naomi requires
-    ## TODO: Some of this is shared between model running and here so we
-    ## should use use common code when we merge this back into hintr.
-    ## This endpoint currently isn't called see mrc-592.


This is unrelated, but just tidied this up whilst I was here

r-ash · 2024-09-09T11:13:19Z

tests/testthat/helper-expect-metadata.R

+    ## This is kind of gross but logic is, if setFilters is not included
+    ## in an effect, then use all the available filters. So have to replicate
+    ## this here for validation


@M-Kusumgar this is true right?

this is not true, https://github.com/mrc-ide/hint/blob/mrc-4856/src/app/static/src/app/store/plotSelections/utils.ts#L59 we initialise filterRefs as empty unless by "available" you mean setFilters on a default effect.

Basically all plots need to have at least one setFilters on the defaultEffect or one of the options in a plot control otherwise nothing shows up on the UI

Thanks! Should have looked at that code, thanks for the link. I've updated the test using proper information from here. This has simplified this function a bit too!

These are failing for annoying to diagnose reasons, but we don't actually need this code to run on mac.

M-Kusumgar

Mostly looks good to me! i had a couple of questions just to make sure im understanding this correctly!

M-Kusumgar · 2024-09-09T13:40:56Z

tests/testthat/helper-expect-metadata.R

+    ## This is kind of gross but logic is, if setFilters is not included
+    ## in an effect, then use all the available filters. So have to replicate
+    ## this here for validation


this is not true, https://github.com/mrc-ide/hint/blob/mrc-4856/src/app/static/src/app/store/plotSelections/utils.ts#L59 we initialise filterRefs as empty unless by "available" you mean setFilters on a default effect.

Basically all plots need to have at least one setFilters on the defaultEffect or one of the options in a plot control otherwise nothing shows up on the UI

M-Kusumgar · 2024-09-09T13:48:15Z

tests/testthat/test-metadata.R

+  ## Testthat skips if only calling custom expect function, so add this
+  ## noddy test to force it to run.


wow thats annoying

M-Kusumgar · 2024-09-09T14:04:08Z

tests/testthat/helper-expect-metadata.R

+      if (isTRUE(filter_types[[filter_id]])) {
+        break
+      }


sorry bit confused by this, is this a truthy check on whether the key exists in filter_types and why do we break if the key does exist?

o damn more silly mistakes here. This is supposed to be if use_shape_regions is true for this filter then skip the check of the options

M-Kusumgar · 2024-09-09T14:06:52Z

tests/testthat/helper-expect-metadata.R

+      expect_option_exists(all_option_ids,
+                           effects$setFilterValues[[state_filter_id]],
+                           paste(context, "setFilterValues"),
+                           effect$id)


where is effect$id coming from?

agh this is some old junk from previous loop which I missed. Should be state_filter_id now, thanks.

M-Kusumgar

happy to approve after that debug is removed, had an optional comment about testing the behaviour of the setFilters effect!

M-Kusumgar · 2024-09-10T11:27:43Z

tests/testthat/helper-expect-metadata.R

+      if (isTRUE(filter_types[[filter_id]]$use_shape_regions)) {
+        ## If we're referring to a filter which uses regions from
+        ## shape file, we have no options in metadata. So skip this check.
+        browser()


rogue debug?

M-Kusumgar · 2024-09-10T11:35:52Z

tests/testthat/helper-expect-metadata.R

+  set_filters <- effects$setFilters
+  if (length(effects$setFilters) == 0) {
+    set_filters <- default_effects$setFilters
+  }
+  expect(length(set_filters) > 0,
+         paste(context, "setFilters must be set in plot setting or",
+               "default settings."))


while this is currently true for all plots, this is not necessarily true, feel free to leave this in, but you can technically do more with the metadata.

For example, say there was a plot control A and a plot control B and a default effect C. You can have it so that only A contains a setFilters effect for all its settings and B, C do not contain a setFilters. Then when your test gets to plot control B, it will fail this test but every option from plot control A still specifies setFilters so everything in the UI would be fine

if you want to properly test this behaviour i would go one level up, pull our combined setFilters (combined from all combinations of all available plot controls + default effect) and test that none of those combined setFilters are empty. that would test it out properly but up to you whether you want to do this or not

Thanks, added a note about this (feeling a bit lazy!)

no problem!

Remove a bunch of old code and endpoints

6c549dc

r-ash force-pushed the remove-old-endpoints branch from 0613dc5 to 6c549dc Compare September 6, 2024 12:55

r-ash commented Sep 9, 2024

View reviewed changes

Add tests of new metadata

eb91c31

r-ash force-pushed the remove-old-endpoints branch from 724f7ef to eb91c31 Compare September 9, 2024 11:19

r-ash added 2 commits September 9, 2024 12:22

Run CI on merge into temporary epic branch

03e85a0

Update integration tests

0e7e317

r-ash requested a review from M-Kusumgar September 9, 2024 13:12

Remove macos CI builds

5364451

These are failing for annoying to diagnose reasons, but we don't actually need this code to run on mac.

M-Kusumgar reviewed Sep 9, 2024

View reviewed changes

Improve testing

3d0a4ea

r-ash force-pushed the remove-old-endpoints branch from 8e09d6a to 3d0a4ea Compare September 9, 2024 16:22

r-ash requested a review from M-Kusumgar September 9, 2024 16:31

r-ash mentioned this pull request Sep 10, 2024

Add end to end test for calibration plot hivtools/hint#993

Merged

2 tasks

M-Kusumgar approved these changes Sep 10, 2024

View reviewed changes

Add explanatory note about test, and remove old debug

6b8bf45

r-ash requested a review from M-Kusumgar September 10, 2024 14:26

r-ash merged commit 5582cd8 into tmp-epic-plot-cleanup Sep 10, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove old endpoints, add tests #523

Remove old endpoints, add tests #523

r-ash commented Sep 6, 2024 •

edited

Loading

r-ash Sep 9, 2024

r-ash Sep 9, 2024

M-Kusumgar Sep 9, 2024

r-ash Sep 9, 2024

M-Kusumgar left a comment

M-Kusumgar Sep 9, 2024

M-Kusumgar Sep 9, 2024

M-Kusumgar Sep 9, 2024

r-ash Sep 9, 2024

M-Kusumgar Sep 9, 2024

r-ash Sep 9, 2024

M-Kusumgar left a comment

M-Kusumgar Sep 10, 2024

M-Kusumgar Sep 10, 2024

r-ash Sep 10, 2024

M-Kusumgar Sep 10, 2024

		## Testthat skips if only calling custom expect function, so add this
		## noddy test to force it to run.

Remove old endpoints, add tests #523

Remove old endpoints, add tests #523

Conversation

r-ash commented Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-ash commented Sep 6, 2024 •

edited

Loading