Ticket #10398 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

Missing destructive test scenarios

Reported by: Owen Arnold Owned by: Harry Jeffery
Priority: minor Milestone: Release 3.3
Component: Reflectometry Keywords:
Cc: Blocked By: #10380
Blocking: Tester: Owen Arnold

Description

The changes made following the ​http://trac.mantidproject.org/mantid/ticket/10380#comment:7 were nearly correct on the functional side, but insufficient on the side of the testing, and therefore an opportunity has been missed to consider edge cases prior to marking the work as complete. I have passed 10380 because I have manually tested and checked the happy path case provided in the unit test.

The tests that provide the most value are those that have the best ability to uncover errors. Often, these are not the happy path cases. Take for example my manual testing of the options column: 

Params='1,1,10', WavelengthMin=1, WavelengthMax=5,u=8

Correctly stops processing and gives warning. Great! 

Params='1,1,10', WavelengthMin=1, WavelengthMax=5,

This is allowed?! 

Params='1,1,10', WavelengthMin=1, WavelengthMax=5,u

This is also allowed, even though u is not a permissible key value and there is no value for it.

Even if these scenarios above where valid behaviours (and I don't think they should be). The lack of test coverage to demonstrate this implies that they are undesired.

Extend the unit tests, and use TDD with a few more destructive test (read documented) cases. Then fix the the parsing static method to do what you would expect.

Change History

comment:1 Changed 6 years ago by Owen Arnold

  • Status changed from new to assigned

comment:2 Changed 6 years ago by Owen Arnold

  • Summary changed from Missing destructive test schenarios to Missing destructive test scenarios

comment:3 Changed 6 years ago by Harry Jeffery

  • Status changed from assigned to inprogress

This commit has been deleted.

Last edited 6 years ago by Harry Jeffery (previous) (diff)

comment:4 Changed 6 years ago by Harry Jeffery

This commit has been deleted.

Last edited 6 years ago by Harry Jeffery (previous) (diff)

comment:5 Changed 6 years ago by Harry Jeffery

Refs #10398 Add more aggressive testing

These invalid inputs should throw an exception.

Changeset: 5d9ea12e63ba2cc3944f77e5576312bbe0c3e749

comment:6 Changed 6 years ago by Harry Jeffery

Refs #10398 Update parseKeyValueString to satisfy unit test

Changeset: b94de72e3e6357407de02cc1cf608fd0f021f1dd

comment:7 Changed 6 years ago by Harry Jeffery

  • Status changed from inprogress to verify
  • Resolution set to fixed

Testing

  • Verify tests are passing
  • Inspect changes

comment:8 Changed 6 years ago by Owen Arnold

  • Status changed from verify to verifying
  • Tester set to Owen Arnold

comment:9 Changed 6 years ago by Owen Arnold

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/feature/10398_add_destructive_tests'

Full changeset: ​71de4a94152b38249dd7ae8d9db69249a909f4b9

comment:10 Changed 6 years ago by Harry Jeffery

Refs #10398 Add more aggressive testing

These invalid inputs should throw an exception.

Changeset: 7a5d16582025a018923ef082d273357c28be1dcf

comment:11 Changed 6 years ago by Harry Jeffery

Refs #10398 Update parseKeyValueString to satisfy unit test

Changeset: 126a6ef4469d5760b0a6f2d076a682624da3d81e

comment:12 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue ​11240

Note: See TracTickets for help on using tickets.