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 optional value to fail-fast option #6275

Merged
merged 3 commits into from Dec 11, 2021
Merged

Conversation

Verest
Copy link
Contributor

@Verest Verest commented Nov 9, 2021

#6152

First time working with this code base. This appears to work, although ran into a few issues.

  1. Not super happy about having to cache the suite object in order to potentially stop it later. Is there some form of a container present for a better way to access this?
  2. Had to be a bit hacky around the failure tests. Ended out just making a separate test class/group for multiple failures.
  3. I could not get 5 of the RemoteWith... tests to pass locally. However, I don't think this is related to my changes.

Aside that, let me know if any requested changes or bugs found. Intent was to keep this working with boolean values as well for fail-fast, in terms of the existing configuration files.

Not sure if this falls into the "merge into 4.1" or "merge into master" category, defaulting to 4.1 for the time being. Let me know if it should be changed to master.

@DavertMik
Copy link
Member

Looks good to me! Absolutely needed feature and proper implementation!

@Verest
Copy link
Contributor Author

Verest commented Nov 18, 2021

@DavertMik Well... it somehow is failing with windows-only php8.1. Is this a related failure to my changes?

I am using linux and cannot replicate this error currently. At the very least, this test ( SuiteManagerTest: Few tests) does not fail on linux php8.1.

@DavertMik
Copy link
Member

PHP 8,1 and Windows.. what a combo. It is related because the only test is failing is the one you added.
Well, you can skip this test on PHP 8.1 and Windows for now. Let's re-enable if we get more information on this cause.

@Verest
Copy link
Contributor Author

Verest commented Nov 18, 2021

In retrospect, I am not sure if php8.1 was actually ran via ubuntu based on the details, although still passes for me locally. Technically I suppose php8.1 isn't even released yet 😆 .

Unless I am missing something, I did not modify the test that is failing - it appears tests\unit\Codeception\SuiteManagerTest.php:testFewTests is the culprit. Did you want me to add logic to skip this test if windows/php8.1 is detected?

@DavertMik
Copy link
Member

DavertMik commented Nov 20, 2021

Did you want me to add logic to skip this test if windows/php8.1 is detected?

@Verest Yes, please do

@Verest
Copy link
Contributor Author

Verest commented Nov 23, 2021

@DavertMik all green now~

@Naktibalda Naktibalda merged commit 4573649 into Codeception:4.1 Dec 11, 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

3 participants