-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
revisit the invalid test DiffractionObject #241
Comments
@sbillinge I am trying to resolve this issue - the "signature" here refers to the 4 required init parameters for DiffractionObject? I am trying to understand which test cases might have been invalid |
I had a quick look and I couldn't see any invalid tests that should be valid, so this may already be resolved. When I wrote the comment it seemed that there were some tests failing that were moved to "invalid" rather than fixed so they passed....they had divide by zero etc.,. If we can find the PR number where the original comment was made I can quckly look and make sure it is resolved correctly. |
Found it! #228 (comment) gotta love github |
OK, these are the guys in question:
|
The first one is instantiating with empty arrays.
A potential UC for this might be
We may not want to support this UC but provide a separate function outside the DO, though it kind of makes sense to have it this way too. |
the second one does
The UC for this is
We should think about exactly how to handle clashes, i.e., if the load finds the metadata in the file that is already loaded. |
We can put UC 1 and UC 2 into the 3.7.0 release. They are a bit tricky and related to other serialization issues. But for now it means that we do want to be able to instantiate empty DO's I think, and so we want these tests to be valid. I am not sure if they are valid and tested but perhaps @bobleesj you could check? |
thanks for this - it was a 10-15 min quick fix. Made a PR here! #284 |
Problem
A bunch of tests became invalid when we changed the signature, but I think they were testing behavior that we wanted to be valid
Proposed solution
Check what behavior we want to test.
The text was updated successfully, but these errors were encountered: