Validation if status codes comparators parameters were provided #296

plutasnyy · 2018-07-27T12:45:55Z

I created validation for StatusCodesComparator`s parameters

Description

I added validation and changed test to be consistent with documentation

Motivation and Context

This change provide validation if mandatory arguments were provided

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

I hereby agree to the terms of the AET Contributor License Agreement.

malaskowski

Actually, it looks that this filter works a little different than docs says.
If user does not provide filterRange or filterCodes the default range is checked, which is 0 - 600.
We would like to keep this behavior.

Instead of making validation here, please update Status Codes Comparator docs and describe how it works, that parameters are optional and what's the behavior if none of parameters is defined.

Bonus: what happens if user define both: filterRange and filterCodes?

tkaik · 2018-07-30T12:35:54Z

As discussed with @Skejven - the default range should be changed to 400-600, because filterRange / filterCodes are actually for specifying which status code should be reported as unexpected (i.e. should mark the test case as failed).

…resion in status code filter

malaskowski · 2018-08-01T10:33:13Z

documentation/src/main/wiki/StatusCodesComparator.md

 | `showExcluded` | boolean (default: `true`) | true | Flag that says if excluded codes (see [[Status Codes Data Filters | StatusCodesDataFilters]]) should be displayed in report. By default set to `true`. | no |
+If you provide `filterRange` and `filterCodes`, it will be used as logical sum. It means that `filterRange="400,500"` `filterCodes=501,502` is equivalent to `filterRange="400,502"` but for `filterRange="300,400"` `filterCodes=404` all codes between `401-403` won`t be checked.


Please leave a blank line between the table and that sentence. There is something wrong with layout without it.

Please change this sentence to:

If you provide both filterRange and filterCodes, it will be used as logical sum. It means that:

<status-codes filterRange="400,500" filterCodes="501,502" /> is equivalent to <status-codes filterRange="400,502" />

<status-codes filterRange="300,400" filterCodes="404" /> won't check 401-403 codes.

malaskowski

Please update CHANGELOG according to AET Contributing rules

…eters_were_provided

wiiitek · 2018-08-02T20:55:08Z

integration-tests/test-suite/partials/status-codes.xml

 			<url href="comparators/statuscodes/noneexistingPage.jsp"/>
 		</urls>
 	</test>

+	<test name="F-comparator-StatusCodes-200-success" useProxy="rest">


We have new test added here.
Please update also our integration tests (functional).

…sCodesComparators_parameters_were_provided

malaskowski · 2018-08-03T12:47:19Z

CHANGELOG.md

@@ -10,6 +10,7 @@ All notable changes to AET will be documented in this file.
 ## Unreleased
 **List of changes that are finished but not yet released in any final version.**

+- [PR-296](https://github.com/Cognifide/aet/pull/296) The behavior of StatusCodesComparator was changed


Could you please add more information here?
E.g.

Status Code Comparator check now range 400-600 by default, parameters validation added

wiiitek · 2018-08-05T17:52:14Z

...est/java/com/cognifide/aet/job/common/comparators/statuscodes/StatusCodesComparatorTest.java

@@ -96,7 +96,7 @@ public void setUp() {
  @Test


There are some issues with this test. Could we resolve them in this pull request?
It looks like dataResult mock is never used (or am I wrong?).
Also: it looks strange when whe change from failed to passed. Why is that?

Currently we are not testing much here. It might be a good time to improve this test.

I added some tests. The change from failed to passed is the result of changed logic expression when we have both filterCodes and filterRange - before, we had AND between range and codes, now we have OR ;)

Thank you! That is great! I had some more ideas (pull request #305). Maybe you would like to get some of them into your feature branch?

Definitely! Thank you so much for your help :D

wiiitek · 2018-08-05T18:05:28Z

integration-tests/sanity-functional/src/test/resources/features/filtering.feature

@@ -55,8 +55,8 @@ Feature: Tests Results Filtering
  Scenario: Filtering Tests Results: status-codes
    Given I have opened sample tests report page
    When I search for tests containing "status"
-    Then There are 20 tiles visible
-    And Statistics text contains "20 ( 8 / 0 / 12 / 0 )"
+    Then There are 21 tiles visible


Thank you for this! :)

Your welcome ;)

…eters_were_provided

tkaik · 2018-08-06T10:26:40Z

documentation/src/main/wiki/StatusCodesDataFilters.md

@@ -1,5 +1,7 @@
 #### Status Codes Data Filters

+Data filters will be apply only for codes contained in the `filterange`. If the `filterRange` isn't provided, default range will be used.


Please fix the filterange typo.
What about codes contained in the filterCodes parameter - will data filters be applied for them?

malaskowski

Before merging to master please merge #305 to this branch first.

Ideas for pull request #296

…eters_were_provided

plutasnyy requested a review from tkaik July 27, 2018 12:46

malaskowski suggested changes Jul 30, 2018

View reviewed changes

malaskowski added enhancement wiki labels Jul 30, 2018

plutasnyy added 5 commits July 31, 2018 13:36

added lower bound for filterrange

b04bcf4

changed unit tests to be correct with logic or in status code filter

a02d7b0

changed logical expresion in status codes filter

b168089

added in documentation information about status code comparator

08a7011

changed status-codes suites to work properly with changed logical exp…

e34670c

…resion in status code filter

plutasnyy force-pushed the validation_if_statusCodesComparators_parameters_were_provided branch from fd9caf4 to e34670c Compare July 31, 2018 11:58

malaskowski reviewed Aug 1, 2018

View reviewed changes

plutasnyy added 2 commits August 1, 2018 15:43

Merge branch 'master' into validation_if_statusCodesComparators_param…

3215cd2

…eters_were_provided

updated changelog and documentation

21a4cae

wiiitek reviewed Aug 2, 2018

View reviewed changes

plutasnyy added 2 commits August 3, 2018 13:28

Merge remote-tracking branch 'origin/master' into validation_if_statu…

66a0f83

…sCodesComparators_parameters_were_provided

updated tests

382696c

malaskowski reviewed Aug 3, 2018

View reviewed changes

unit test formatting

ae809bc

wiiitek suggested changes Aug 5, 2018

View reviewed changes

wiiitek reviewed Aug 5, 2018

View reviewed changes

plutasnyy and others added 4 commits August 6, 2018 08:21

updated documentation

640ed48

removed unused mock

fa61147

updated tests

ecee6ff

Merge branch 'master' into validation_if_statusCodesComparators_param…

2048de5

…eters_were_provided

tkaik reviewed Aug 6, 2018

View reviewed changes

plutasnyy added 3 commits August 6, 2018 12:50

changed urls

618d9fe

fixed typo in documentation

e45fa42

updated documentation

55a4c30

wiiitek added 3 commits August 7, 2018 09:17

more refactoring for unit test

79a1fbb

changelog updated

4cbc564

documentation updates

fc7b234

malaskowski approved these changes Aug 7, 2018

View reviewed changes

Merge pull request #305 from Cognifide/unit-test-refactor

d3c23f9

Ideas for pull request #296

wiiitek approved these changes Aug 7, 2018

View reviewed changes

Merge branch 'master' into validation_if_statusCodesComparators_param…

eb43b1f

…eters_were_provided

tkaik merged commit ff295e4 into master Aug 9, 2018

tkaik deleted the validation_if_statusCodesComparators_parameters_were_provided branch August 9, 2018 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation if status codes comparators parameters were provided #296

Validation if status codes comparators parameters were provided #296

plutasnyy commented Jul 27, 2018

malaskowski left a comment

tkaik commented Jul 30, 2018

malaskowski Aug 1, 2018

malaskowski left a comment

wiiitek Aug 2, 2018

malaskowski Aug 3, 2018 •

edited

Loading

wiiitek Aug 5, 2018 •

edited

Loading

plutasnyy Aug 6, 2018

wiiitek Aug 7, 2018

plutasnyy Aug 7, 2018

wiiitek Aug 5, 2018

plutasnyy Aug 6, 2018

tkaik Aug 6, 2018

plutasnyy Aug 6, 2018

malaskowski left a comment

		\| `showExcluded` \| boolean (default: `true`) \| true \| Flag that says if excluded codes (see [[Status Codes Data Filters \| StatusCodesDataFilters]]) should be displayed in report. By default set to `true`. \| no \|
		If you provide `filterRange` and `filterCodes`, it will be used as logical sum. It means that `filterRange="400,500"` `filterCodes=501,502` is equivalent to `filterRange="400,502"` but for `filterRange="300,400"` `filterCodes=404` all codes between `401-403` won`t be checked.

		@@ -1,5 +1,7 @@
		#### Status Codes Data Filters

		Data filters will be apply only for codes contained in the `filterange`. If the `filterRange` isn't provided, default range will be used.

Validation if status codes comparators parameters were provided #296

Validation if status codes comparators parameters were provided #296

Conversation

plutasnyy commented Jul 27, 2018

Description

Motivation and Context

Screenshots (if appropriate):

Types of changes

Checklist:

malaskowski left a comment

Choose a reason for hiding this comment

tkaik commented Jul 30, 2018

Choose a reason for hiding this comment

malaskowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malaskowski Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

wiiitek Aug 5, 2018 • 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

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

malaskowski left a comment

Choose a reason for hiding this comment

malaskowski Aug 3, 2018 •

edited

Loading

wiiitek Aug 5, 2018 •

edited

Loading