Skip to content
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

Behavioral tests for Tax commands #14132

Merged
merged 17 commits into from Sep 2, 2019

Conversation

@zuk3975
Copy link
Contributor

commented Jun 10, 2019

Questions Answers
Branch? develop
Description? Adds behavioral tests for tax create/edit/delete actions. Also adds missing typecasting in GetTaxForEditingHandler
Type? refacto
Category? TE
BC breaks? no
Deprecations? no
Fixed ticket? #14480
How to test? Tests can be executed by command: ./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s tax

This change is Reviewable

@zuk3975 zuk3975 requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 10, 2019
@zuk3975 zuk3975 changed the title [WIP] Tax management behat tests [WIP] Behavioral tests for tax commands Jun 10, 2019
@zuk3975 zuk3975 changed the title [WIP] Behavioral tests for tax commands [WIP] Behavioural tests for tax commands Jun 20, 2019
@matks matks added the migration label Jun 24, 2019
@zuk3975 zuk3975 force-pushed the zuk3975:behat/tax branch from 7b32e2e to b155362 Jun 25, 2019
@zuk3975 zuk3975 changed the title [WIP] Behavioural tests for tax commands Behavioural tests for tax commands Jun 25, 2019
@zuk3975 zuk3975 changed the title Behavioural tests for tax commands [WIP] Behavioural tests for tax commands Jul 16, 2019
@zuk3975 zuk3975 changed the title [WIP] Behavioural tests for tax commands [WIP] Behavioural tests for Tax commands Jul 17, 2019
@zuk3975 zuk3975 changed the title [WIP] Behavioural tests for Tax commands [WIP] Behavioral tests for Tax commands Jul 17, 2019
@zuk3975 zuk3975 force-pushed the zuk3975:behat/tax branch from 13d0cd4 to 352ea75 Jul 17, 2019
@zuk3975 zuk3975 changed the title [WIP] Behavioral tests for Tax commands Behavioral tests for Tax commands Jul 17, 2019
$expectedStatus = 'enable' === $action ? true : false;
$this->getCommandBus()->handle(new BulkToggleTaxStatusCommand(
array_map('intval', explode(',', $ids)),

This comment has been minimized.

Copy link
@matks

matks Jul 22, 2019

Contributor

Tricky but funny 👍

And tax "tax-1" rate should be 0.150
And tax "tax-1" should be disabled

Scenario: Editing tax when providing only changeable property should be allowed

This comment has been minimized.

Copy link
@matks

matks Jul 22, 2019

Contributor
Suggested change
Scenario: Editing tax when providing only changeable property should be allowed
Scenario: It is possible to modify only the name of a tax, nothing else

Let's write very story-like scenarios 😄

When I edit tax "tax-1" with following properties:
| name | my custom tax 300 |
| rate | 0.15 |
| is_enabled | 0 |

This comment has been minimized.

Copy link
@matks

matks Jul 22, 2019

Contributor

If you use PrimitiveUtils::castStringBooleanIntoBoolean() on this table row, you can write

Suggested change
| is_enabled | 0 |
| is_enabled | 0 |

which is easier to read

Then tax "tax-1" should be enabled

Scenario: Disabling taxes status in bulk action
Given taxes with ids: "1,2,3" exists

This comment has been minimized.

Copy link
@matks

matks Jul 22, 2019

Contributor

Mmmm 😐 it's sad we have a lot of good scenarios talking about tax references (like tax-1 although it could be interesting to use names even closer to reality like "TVA", "State Tax", "IRS tax") and suddenly we have scenarios talking about IDs (which are technical-oriented)

Cant we create 3 taxes before using the bulk ?

When I add new tax "tax-1" with following properties:
      | name         | my custom tax 501 |
      | rate         | 0.5               |
      | is_enabled   | 1                 |
And When I add new tax "tax-2" with following properties:
      | name         | my custom tax 502 |
      | rate         | 0.6               |
      | is_enabled   | 1                 |
And I add new tax "tax-3" with following properties:
      | name         | my custom tax 503 |
      | rate         | 0.7               |
      | is_enabled   | 1                 |
And I disable the following taxes: "tax-1", "tax-2", "tax-3"

And we retrieve the IDs required for the Command from the tax references

When I enable taxes with ids: "1,2,3"
Then taxes with ids: "1,2,3" should be enabled

Scenario: Deleting tax

This comment has been minimized.

Copy link
@matks

matks Jul 22, 2019

Contributor

You can mix some actions together to avoid having too small scenarios 😉 it's authorized as it's integration tests. It's actually interesting to see whether a test works well when paired with another. So we can for example merge the "Toggling tax status" with the "Deleting tax" (run one after the other) or "Disabling taxes status in bulk action" with "Deleting taxes in bulk action"

We can also aim for more user-oriented scenario titles:
Deleting taxes in bulk action => Deleting multiple taxes from a BO listing

@sarjon

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019


tests/Integration/Behaviour/Features/Context/Domain/TaxFeatureContext.php, line 96 at r3 (raw file):


    /**
     * @When I toggle tax :taxReference status

When toggling you won't know the end result. How about I enable/disable tax ...?

@sarjon

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019


tests/Integration/Behaviour/Features/Context/Domain/TaxFeatureContext.php, line 150 at r3 (raw file):

    {
        foreach (PrimitiveUtils::castStringArrayIntoArray($taxReferences) as $taxReference) {
            $this->deleteTax($taxReference);

Shouldn't you use bulk delete command instead?

Copy link
Member

left a comment

Reviewed 1 of 3 files at r1, 1 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @zuk3975)


tests/Integration/Behaviour/Features/Context/Domain/TaxFeatureContext.php, line 160 at r3 (raw file):

    {
        foreach (PrimitiveUtils::castStringArrayIntoArray($taxReferences) as $taxReference) {
            $this->assertTaxisDeleted($taxReference);

TaxIsDeleted


tests/Integration/Behaviour/Features/Context/Domain/TaxFeatureContext.php, line 167 at r3 (raw file):

     * @Then tax :taxReference should be deleted
     */
    public function assertTaxisDeleted($taxReference)

Same here, "is" camel case?


tests/Integration/Behaviour/Features/Context/Domain/TaxFeatureContext.php, line 224 at r3 (raw file):

    public function assertTaxesExistsByIds($ids)
    {
        foreach (explode(',', $ids) as $id) {

How about using PrimitiveUtils?


tests/Integration/Behaviour/Features/Scenario/Tax/tax_management.feature, line 4 at r3 (raw file):

#./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s tax
Feature: Manage tax
  As a employee

As an employee


tests/Integration/Behaviour/Features/Scenario/Tax/tax_management.feature, line 5 at r3 (raw file):

Feature: Manage tax
  As a employee
  I must be able to correctly add, edit and delete tax

Not sure "correctly" says anything.


tests/Integration/Behaviour/Features/Scenario/Tax/tax_management.feature, line 33 at r3 (raw file):

  Scenario: Toggling tax status
    Given tax "sales-tax" should be disabled

"should be" gives impression that it can also be enabled. How about Given tax "sales-tax" is disabled?


tests/Integration/Behaviour/Features/Scenario/Tax/tax_management.feature, line 43 at r3 (raw file):

    Then tax "sales-tax" should be deleted

  Scenario: Disabling multiple taxes status in bulk action

You can remove word "status" imo.


tests/Integration/Behaviour/Features/Scenario/Tax/tax_management.feature, line 58 at r3 (raw file):

    When I disable taxes: "beard-tax, state-tax, pvm-tax"
    Then taxes: "beard-tax, state-tax" should be disabled
    And tax "pvm-tax" should be disabled

What's the point of checking this tax separately?

…method and redundant code
@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@matks addressed your comments. Travis is mad, he is dying to see #blockcart-modal (Error: element (#blockcart-modal) still not visible after 2500ms), which i guess is not up to me.

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

@matks addressed your comments. Travis is mad, he is dying to see #blockcart-modal (Error: element (#blockcart-modal) still not visible after 2500ms), which i guess is not up to me.

Yes, these tests are flawed 😞
I'm working hard to convince my teammates to drop them but I have not yet succeeded 😛

Re-running build ...

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@matks last requested changes were applied. Could you please restart travis once again to merge this?

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Yes, these tests are flawed 😞
I'm working hard to convince my teammates to drop them but I have not yet succeeded 😛

Note: I won

#15161

@matks
matks approved these changes Aug 20, 2019
@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Now PrettyCI job status has been lost 😡

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

@zuk3975 All CI checks are now 💚but we need a rebase to fix git conflicts ... 😞

@zuk3975

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

@zuk3975 All CI checks are now but we need a rebase to fix git conflicts ...

@matks, resolved conflicts, prettyCI seems to be broken again 😄

@matks
matks approved these changes Sep 2, 2019
@matks matks added this to the 1.7.7.0 milestone Sep 2, 2019
@matks

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

Thank you @zuk3975

@matks matks merged commit d2b7171 into PrestaShop:develop Sep 2, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 3 files, 16 discussions left (sarjon, zuk3975)
Details
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.