Skip to content

test: uploaded files #5

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

Merged
merged 15 commits into from
Aug 2, 2021
Merged

test: uploaded files #5

merged 15 commits into from
Aug 2, 2021

Conversation

fkupper
Copy link
Contributor

@fkupper fkupper commented Jul 31, 2021

@fkupper fkupper requested a review from TavoNiievez July 31, 2021 21:57
@TavoNiievez
Copy link
Member

@fkupper It seems you forgot a use statement in your PR in module-laravel, just like here. I fixed it with Codeception/module-laravel#27 .

I changed another assertEquals to assertSame b603cc2.

Finally, i ran these tests locally (on Windows) and they passed, so there must be a problem with the CI config. I tried to fix it with fc332eb with no success.

@fkupper
Copy link
Contributor Author

fkupper commented Aug 2, 2021

@TavoNiievez I've added two changes to help understand the error.

  • Fail the test in case json decoding fails
  • Temporary run codeception with -d to check what is the output on CI

@fkupper
Copy link
Contributor Author

fkupper commented Aug 2, 2021

@TavoNiievez can you remove my restriction to run CI tests at least for the duration of this PR?

@TavoNiievez
Copy link
Member

@fkupper I'm not sure how to do that (or if possible). I think GitHub made that change because some people were taking advantage of builds to mine cryptocurrencies.

Have you tried activating Actions on your fork?

@TavoNiievez
Copy link
Member

It seems that that was the problem, the last execution was correct, the only thing that generates a little doubt is knowing if disableMiddleware() deactivates all Middlewares, is there a way to specify that you only want to deactivate \App\Http\Middleware\VerifyCsrfToken ?

@fkupper
Copy link
Contributor Author

fkupper commented Aug 2, 2021

It seems that that was the problem, the last execution was correct, the only thing that generates a little doubt is knowing if disableMiddleware() deactivates all Middlewares, is there a way to specify that you only want to deactivate \App\Http\Middleware\VerifyCsrfToken ?

What do you think of using and api route instead of a web route? The api route group have no CSRF middleware.

@fkupper
Copy link
Contributor Author

fkupper commented Aug 2, 2021

What do you think of using and api route instead of a web route? The api route group have no CSRF middleware.

I've pushed the changes with this suggestion so you can have a look.

Comment on lines 17 to 22
$response = json_decode($response, true);

$last_error = json_last_error();
if ($last_error !== JSON_ERROR_NONE) {
$this->fail("Failed to parse response from uploaded-files endpoint with json error code {$last_error}");
}
Copy link
Member

Choose a reason for hiding this comment

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

One last thing.

Since we are on PHP 7.3+, how about using JSON_THROW_ON_ERROR instead?

See this ref link.

@fkupper fkupper requested a review from TavoNiievez August 2, 2021 15:43
@TavoNiievez TavoNiievez merged commit 8540164 into Codeception:main Aug 2, 2021
@TavoNiievez
Copy link
Member

@fkupper good work!

thanks for your patience in this matter.

@fkupper
Copy link
Contributor Author

fkupper commented Aug 2, 2021

@fkupper good work!

thanks for your patience in this matter.

You're welcome!
My pleasure.

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.

2 participants