Bug 15339

Summary: TestBuilder build parameter warnings
Product: Koha Reporter: Martin Persson <xarragon>
Component: Test SuiteAssignee: Martin Persson <xarragon>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19268    
Bug Blocks: 22930    
Attachments: Bug 15339: TestBuilder warnings (code)
Bug 15339: TestBuilder warnings (tests)
Bug 15339: TestBuilder warnings (code)
Bug 15339: TestBuilder warnings (tests)
Bug 15339: Improve parameter validation
Bug 15339: TestBuilder warnings (code)
Bug 15339: TestBuilder warnings (tests)
Bug 15339: Improve parameter validation
Bug 15339: [QA Follow-up] Add a warning too when source is not supplied
Bug 15339: Remove extra 'my'

Description Martin Persson 2015-12-09 12:05:09 UTC
TestBuilder's build function ignores any parameters that it does not recognize. If the developer forgets to wrap the column values in a 'value' hashref he or she might waste time checking the code rather than the tests. This patch makes TestBuilder issue a warning whenever unregonized parameters are passed to build.
Comment 1 Martin Persson 2015-12-09 12:27:03 UTC
Created attachment 45527 [details] [review]
Bug 15339: TestBuilder warnings (code)

Makes TestBuilder::build() alert the user when unreognized
parameters are passed, which happens when the user supplies
the column values directly, forgetting the 'value' hash.

This patch holds the code changes. Examples of the kind of
errors that it catches are in the tests (separate patch).

Sponsored-By: Halland County Library
Comment 2 Martin Persson 2015-12-09 12:27:20 UTC
Created attachment 45528 [details] [review]
Bug 15339: TestBuilder warnings (tests)

Makes TestBuilder::build() alert the user when unreognized parameters
are passed, which happens when the user supplies the column values
directly, forgetting the 'value' hash.

This patch contains the tests that doubles as a demonstration
of the kind of error the patch is intended to prevent.

Sponsored-By: Halland County Library
Comment 3 Martin Persson 2015-12-09 20:07:17 UTC
Realized there are some shortcomings that I need to fix before this is worth looking at.
Comment 4 Jonathan Druart 2017-09-06 16:43:10 UTC
Created attachment 66892 [details] [review]
Bug 15339: TestBuilder warnings (code)

Makes TestBuilder::build() alert the user when unreognized
parameters are passed, which happens when the user supplies
the column values directly, forgetting the 'value' hash.

This patch holds the code changes. Examples of the kind of
errors that it catches are in the tests (separate patch).

Sponsored-By: Halland County Library

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Jonathan Druart 2017-09-06 16:43:14 UTC
Created attachment 66893 [details] [review]
Bug 15339: TestBuilder warnings (tests)

Makes TestBuilder::build() alert the user when unreognized parameters
are passed, which happens when the user supplies the column values
directly, forgetting the 'value' hash.

This patch contains the tests that doubles as a demonstration
of the kind of error the patch is intended to prevent.

Sponsored-By: Halland County Library

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Jonathan Druart 2017-09-06 16:43:18 UTC
Created attachment 66894 [details] [review]
Bug 15339: Improve parameter validation

Only value and source are allowed

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Jonathan Druart 2017-09-06 16:43:49 UTC
Patches have been rebased and adapt to the new code.
Comment 8 Marcel de Rooy 2017-09-07 10:09:58 UTC
Created attachment 66921 [details] [review]
Bug 15339: TestBuilder warnings (code)

Makes TestBuilder::build() alert the user when unreognized
parameters are passed, which happens when the user supplies
the column values directly, forgetting the 'value' hash.

This patch holds the code changes. Examples of the kind of
errors that it catches are in the tests (separate patch).

Sponsored-By: Halland County Library

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-09-07 10:10:06 UTC
Created attachment 66922 [details] [review]
Bug 15339: TestBuilder warnings (tests)

Makes TestBuilder::build() alert the user when unreognized parameters
are passed, which happens when the user supplies the column values
directly, forgetting the 'value' hash.

This patch contains the tests that doubles as a demonstration
of the kind of error the patch is intended to prevent.

Sponsored-By: Halland County Library

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-09-07 10:10:11 UTC
Created attachment 66923 [details] [review]
Bug 15339: Improve parameter validation

Only value and source are allowed

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-09-07 10:10:16 UTC
Created attachment 66924 [details] [review]
Bug 15339: [QA Follow-up] Add a warning too when source is not supplied

When you do not supply a source and add a few wrong parameters, you
would not be warned. Because build simply returns undef.
Adding a carp and a test for that situation too.

Note: In the earlier subtest 'trivial tests' build was called without
source. This now generates a warning. We just catch if there is a warning
and test the actual warning itself later on.

Test plan:
Run t/db_dependent/TestBuilder.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Jonathan Druart 2017-09-07 16:55:26 UTC
Created attachment 66951 [details] [review]
Bug 15339: Remove extra 'my'
Comment 13 Jonathan Druart 2017-09-07 18:06:40 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 14 Marcel de Rooy 2017-09-08 05:37:56 UTC
(In reply to Jonathan Druart from comment #12)
> Created attachment 66951 [details] [review] [review]
> Bug 15339: Remove extra 'my'

O my. Thx for catching that..
Comment 15 Fridolin Somers 2017-10-10 09:50:59 UTC
Enhancement not pushed to 17.05.x