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

Added travis + tests + cs fixes #24

Merged
merged 5 commits into from
Oct 18, 2018
Merged

Added travis + tests + cs fixes #24

merged 5 commits into from
Oct 18, 2018

Conversation

mickaelandrieu
Copy link
Contributor

@mickaelandrieu mickaelandrieu commented Oct 16, 2018

This contribution is about adding premises of a test suite and coding styles.

Also, a configuration for Travis.

@@ -19,7 +19,7 @@
* needs please refer to http://www.prestashop.com for more information.
*
* @author PrestaShop SA <contact@prestashop.com>
* @copyright 2007-2016 PrestaShop SA
* @copyright 2007-2016 PrestaShop SA
Copy link
Contributor

Choose a reason for hiding this comment

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

2016?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I update this one I need to update all the others, I'd like to keep this pull request with one scope only

.travis.yml Outdated Show resolved Hide resolved
"scripts": {
"cs-fix": "@php ./vendor/bin/php-cs-fixer fix .",
"cs-fix-test": "@php ./vendor/bin/php-cs-fixer fix --dry-run --stop-on-violation --show-progress=dot .",
"test": "SYMFONY_PHPUNIT_VERSION=5.7 php ./vendor/bin/simple-phpunit"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting point here: passing env variables here make the special @php unavailable in Composer scripts. I don't know if it's intended or not.

@matks
Copy link
Contributor

matks commented Oct 18, 2018

@mickaelandrieu Wen being shipped to Addons as a downloadable PS module, do you know what process is performed ? I think there is at least 1 composer install.

  • is the composer install performed for DEV or PROD
  • shall we remove the test files (phpunit.xml, .travis.yml) and folders (tests and tools) as they are not useful for the module to be executed/installed ?

@matks
Copy link
Contributor

matks commented Oct 18, 2018

⚠️ this PR targets master instead of dev, is that intended ?

@mickaelandrieu
Copy link
Contributor Author

mickaelandrieu commented Oct 18, 2018

is the composer install performed for DEV or PROD?

I don't think composer is called at all, that's why I don't use it to autoload module classes

shall we remove the test files (phpunit.xml, .travis.yml) and folders (tests and tools) as they are not useful for the module to be executed/installed ?

We could, but how? I'm thinking about using deploy feature of travis ci to perform these tasks, but this wont be addressed here.

this PR targets master instead of dev, is that intended ?

I don't know because I can't figure out why we have both dev and master branches on every module :/ ping @Quetzacoalt91

@Quetzacoalt91
Copy link
Contributor

Quetzacoalt91 commented Oct 18, 2018

  • Composer is run with --no-dev parameter when deployed to the marketplace. This allow the zip to not provide useless stuff for the merchants.

  • dev contains the current version in progress. Once we are ready to publish a new version, we merge dev into master to trigger a new release on the marketplace the next night.

@mickaelandrieu mickaelandrieu changed the base branch from master to dev October 18, 2018 11:42
@mickaelandrieu
Copy link
Contributor Author

I've switched to dev and as I have only dev dependencies, there won't be any impact for the marketplace.

Thanks for the answers @Quetzacoalt91 !

@matks matks merged commit 6756078 into PrestaShopCorp:dev Oct 18, 2018
@matks
Copy link
Contributor

matks commented Oct 18, 2018

Thanks @mickaelandrieu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants