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

fix: uploaded files should have test flag set to true #26

Merged
merged 3 commits into from
Jul 31, 2021

Conversation

fkupper
Copy link
Contributor

@fkupper fkupper commented Jul 30, 2021

The block of code fixing the issue Codeception/Codeception#3417 was remove when moving from laravel 5 module.

I do not know what was the reasoning behind it, but it breaks tests relying on the test flag being set to true.

I did not add any tests as this change is not fitting your contributor's guide and it is unclear to me where to do this.

@TavoNiievez
Copy link
Member

TavoNiievez commented Jul 30, 2021

@fkupper Whoops, that was me. (here #7)
I probably wrongly assumed that the fact that CI worked meant that it did not affect recent versions of Laravel.

I would like to apologize for the fact that the contribution guides are not so clear, I had yet to update them with respect to the module-symfony ones (where I am usually more active as a contributor).

I can solve all the doubts you have regarding the contribution guides in order to improve them in the process.

But basically:

  1. I would need a PR in laravel-module-tests where, through a test failing, the CI of that repo confirms the issue.
  2. I merge this PR with the patch to the main branch.
  3. I re-run the tests of the PR from step 1 and the error should go away. That PR is merged.
  4. I do a tag release of your fix. (rather, I add again what janhenkgerritsen did in Laravel5 fix issue with uploaded files Codeception#3429 ideally with a co-authored commit)

fkupper and others added 2 commits July 30, 2021 22:10
Co-authored-by: Tavo Nieves J <64917965+TavoNiievez@users.noreply.github.com>
@fkupper
Copy link
Contributor Author

fkupper commented Jul 30, 2021

@fkupper Whoops, that was me. (here #7)
I probably wrongly assumed that the fact that CI worked meant that it did not affect recent versions of Laravel.

This is why tests are important. I really wanted to add a test for this but it doesn't fit the laravel-module-tests IMHO.

I would like to apologize for the fact that the contribution guides are not so clear, I had yet to update them with respect to the module-symfony ones (where I am usually more active as a contributor).

Please, no need for apologize! The guide is clear enough. What I mean is that the guide intends to test the module's methods and not the underlying code, therefore I did not see a place where the tests would fit.
If you have a strong opinion of how to introduce these tests, just point the way and I can add it, even if it is completely new test suit or something.

I can solve all the doubts you have regarding the contribution guides in order to improve them in the process.

But basically:

1. I would need a PR in [`laravel-module-tests`](https://github.com/Codeception/laravel-module-tests) where, through a test failing, the CI of that repo confirms the issue.

2. I merge this PR with the patch to the `main` branch.

3. I re-run the tests of the PR from step 1 and the error should go away. That PR is merged.

4. I do a tag release of your fix. (rather, I add again what **janhenkgerritsen** did in [Laravel5 fix issue with uploaded files Codeception#3429](https://github.com/Codeception/Codeception/pull/3429) ideally with a co-authored commit)

⬆️ This is very clear from the guide, don't worry! ;)

@fkupper
Copy link
Contributor Author

fkupper commented Jul 30, 2021

BTW, thanks for the effort of writing a proper reply with clear and meaningful content. 👏

@TavoNiievez
Copy link
Member

TavoNiievez commented Jul 30, 2021

Create a new test suite called IssuesCest and add a link to your PR in the dockblock. (eg. IssuesCest)

@fkupper
Copy link
Contributor Author

fkupper commented Jul 30, 2021

Create a new test suite called IssuesCest and add a link to your PR in the dockblock

Thank you!
I will have a look tomorrow, it is late in my timezone :)

@fkupper
Copy link
Contributor Author

fkupper commented Jul 31, 2021

Create a new test suite called IssuesCest and add a link to your PR in the dockblock. (eg. IssuesCest)

Tests added ✅
Codeception/laravel-module-tests#5

@TavoNiievez TavoNiievez merged commit 7ef1bbd into Codeception:main Jul 31, 2021
@TavoNiievez
Copy link
Member

@fkupper I released your changes as version 2.0.1.

@fkupper
Copy link
Contributor Author

fkupper commented Aug 2, 2021

Thank you so much.

@TavoNiievez TavoNiievez added this to the 2.0.1 milestone Sep 10, 2021
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

2 participants