Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-34616

Avoid NULL/None for implied dimensions in test data IDs

    XMLWordPrintable

    Details

      Description

      The addition of a NOT NULL constraint on DM-21840 is a schema change (albeit a very small one) that needs to merged alongside a migration script. But we can fix the tests that it will break in advance to avoid further complicating that big merge, and that's what this ticket is for.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Krzysztof Findeisen, mind reviewing this set of small changes to butler tests and test utility code, and some downstream tests that use the latter?

            The bottom line is that DM-21840 is soon going to add some NOT NULL constraints for implied dimensions, and that breaks test code that has thus far gotten away with e.g. declaring visit data IDs without any filters. I think you'll agree that having the test code this dependent on the dimensions schema is not ideal, at least for testing code that doesn't depend on the data IDs it's given (of course, some might), but I also don't see any real alternatives (short of making the test utility code much more reliant on mocks), and at least the fixes are small.

            Changes are spread across multiple packages. I identified these packages as needing fixes by running Jenkins on DM-21840 and seeing what broke; I then just rebased the test fixes to this ticket.

            Show
            jbosch Jim Bosch added a comment - Krzysztof Findeisen , mind reviewing this set of small changes to butler tests and test utility code, and some downstream tests that use the latter? The bottom line is that DM-21840 is soon going to add some NOT NULL constraints for implied dimensions, and that breaks test code that has thus far gotten away with e.g. declaring visit data IDs without any filters. I think you'll agree that having the test code this dependent on the dimensions schema is not ideal, at least for testing code that doesn't depend on the data IDs it's given (of course, some might), but I also don't see any real alternatives (short of making the test utility code much more reliant on mocks), and at least the fixes are small. Changes are spread across multiple packages. I identified these packages as needing fixes by running Jenkins on DM-21840 and seeing what broke; I then just rebased the test fixes to this ticket. daf_butler pipe_base obs_base verify pipe_tasks
            Hide
            jbosch Jim Bosch added a comment -

            Krzysztof Findeisen, I think this is ready for another look. I've addressed the big problem with the original change: the test utility code now invents data ID values for dependencies when they're needed. The changes to daf_butler were more substantial than our discussion on the PR predicted, but the PRs for most other packages have been closed and their branches deleted. The pipe_tasks one is still there because it didn't use the test utility code and hence still needs more manual dimension record insertions to be compatible with the upcoming NOT NULL constraint.

            Show
            jbosch Jim Bosch added a comment - Krzysztof Findeisen , I think this is ready for another look. I've addressed the big problem with the original change: the test utility code now invents data ID values for dependencies when they're needed. The changes to daf_butler were more substantial than our discussion on the PR predicted, but the PRs for most other packages have been closed and their branches deleted. The pipe_tasks one is still there because it didn't use the test utility code and hence still needs more manual dimension record insertions to be compatible with the upcoming NOT NULL constraint.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Thank you for your patience, I'm fine with these changes now.

            Show
            krzys Krzysztof Findeisen added a comment - Thank you for your patience, I'm fine with these changes now.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Krzysztof Findeisen
              Watchers:
              Jim Bosch, Krzysztof Findeisen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.