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

MSI-1411: Add new rule to copy paste detector blacklist to avoid cove… #16508

Conversation

VitaliyBoyko
Copy link
Contributor

@VitaliyBoyko VitaliyBoyko commented Jul 3, 2018

Description

In the MSI modules all fixtures for tests are in <Module_Name>/Test/_data which is different from Magento current implementations.
In order to avoid falling copy paste detector tests on the fixtures has been added a new rule to the blacklist.

Fixed Issues (if relevant)

Static test falling

PS. It's very important for MSI ;)

@magento-engcom-team magento-engcom-team added Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Jul 3, 2018
@magento-engcom-team
Copy link
Contributor

Hi @VitaliyBoyko. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@orlangur
Copy link
Contributor

orlangur commented Jul 3, 2018

@VitaliyBoyko could you give more insight why do we need copy-pasted fixtures? Also, any contribution must be delivered to 2.2-develop first, no matter MSI-related or not.

@VitaliyBoyko
Copy link
Contributor Author

Hi @orlangur
Thank you for review.

MSI modules have own fixtures which are similar to core ones. Similar, but not exactly the same. Including code from core gonna bring more complexity than we suppress possible copypaste parts.

any contribution must be delivered to 2.2-develop
There is some rule in Contribution Guide? I could not found it :(
Could you please share these requirements or just explain why we need changes related only to 2.3 in 2.2 develop.

Thank you!

@ishakhsuvarov
Copy link
Contributor

Hi @orlangur @VitaliyBoyko
I think we can safely deliver this to 2.2. There is not hard requirement in place regarding merge and delivery sequence when pull request is submitted to 2.3.

@magento-engcom-team
Copy link
Contributor

Hi @ishakhsuvarov, thank you for the review.
ENGCOM-2180 has been created to process this Pull Request

@VitaliyBoyko
Copy link
Contributor Author

Hi @ishakhsuvarov
Deal. Thank you!

@orlangur
Copy link
Contributor

orlangur commented Jul 4, 2018

@ishakhsuvarov,

There is not hard requirement in place regarding merge and delivery sequence when pull request is submitted to 2.3.

At least some time ago the process was to put 2.3 contribution on hold until it is processed for 2.2.

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to discussion in Slack: fixtures are full of copy-paste but it is not possible to refactor them quickly.

@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-2180 has been created to process this Pull Request

@orlangur
Copy link
Contributor

orlangur commented Jul 4, 2018

@VitaliyBoyko,

why we need changes related only to 2.3 in 2.2 develop.

It is important to have in 2.3 rules which are equally or more strict than in 2.2 for various static tests. If we won't port it into 2.2 somebody may need to perform an unneeded refactoring of fixture not knowing it is simply ignored in 2.3.

@ishakhsuvarov
Copy link
Contributor

ishakhsuvarov commented Jul 4, 2018

@orlangur Currently the whole dev directory, which contains integration text fixtures is blacklisted. Thus, creating another location for fixtures and creating a blacklist for it makes it equally strict.

UPD: Not that I am against porting this solution. I am just not seeing a point in requiring this PR to be merged in 2.2 first.

@orlangur
Copy link
Contributor

orlangur commented Jul 4, 2018

Thus, creating another location for fixtures and creating a blacklist for it makes it equally strict.

What I meant is that without porting 2.3 would be less strict.

a point in requiring this PR to be merged in 2.2 first.

It is a generic rule, isn't it? Anything applicable to 2.2 should be merged there first.

@magento-engcom-team magento-engcom-team merged commit 7abdafd into magento:2.3-develop Jul 4, 2018
magento-engcom-team pushed a commit that referenced this pull request Jul 4, 2018
@magento-engcom-team
Copy link
Contributor

Hi @VitaliyBoyko. Thank you for your contribution.
We will aim to release these changes as part of 2.3.0.
Please check the release notes for final confirmation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants