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

lang('Auth.errorPasswordEmpty') fails sometime #88

Closed
kenjis opened this issue Apr 29, 2022 · 7 comments · Fixed by #163
Closed

lang('Auth.errorPasswordEmpty') fails sometime #88

kenjis opened this issue Apr 29, 2022 · 7 comments · Fixed by #163

Comments

@kenjis
Copy link
Member

kenjis commented Apr 29, 2022

From #71 (comment)

There was 1 failure:

1) Tests\Unit\PasswordsTest::testEmptyPassword
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'A Password is required.'
+'Auth.errorPasswordEmpty'

/home/runner/work/shield/shield/tests/Unit/PasswordsTest.php:22

https://github.com/codeigniter4/shield/runs/6222433930?check_suite_focus=true

Ref lonnieezell/myth-auth#494 (comment)

@kenjis kenjis changed the title lang('Auth.errorPasswordEmpty') fails lang('Auth.errorPasswordEmpty') fails Apr 29, 2022
@kenjis kenjis changed the title lang('Auth.errorPasswordEmpty') fails lang('Auth.errorPasswordEmpty') fails sometime Apr 29, 2022
@MGatner
Copy link
Member

MGatner commented Apr 29, 2022

While we're collecting evidence I will add that I see this occasionally across many repos that use the framework. Not sure if it is some testing behavior or a bug in the repo itself.

@kenjis
Copy link
Member Author

kenjis commented May 23, 2022

I see this error every day lately.
But I have no clue. After the error occurs, the test passes normally and it never happens twice in a row.

@MGatner
Copy link
Member

MGatner commented May 23, 2022

I know, it's a pain. It happens elsewhere so definitely not something specific to this repo.

Here's what I feel fairly certain about: the error only happens erratically because we run tests in random order, therefor the underlying issue has something to do with ordering of service interactions.

That said, because it is random and rare it is very hard to reproduce the scenario that caused the failure to troubleshoot. My hunch is that it is related to Services.

@kenjis
Copy link
Member Author

kenjis commented May 23, 2022

executionOrder="random"!
I didn't know it.

@MGatner
Copy link
Member

MGatner commented May 23, 2022

It's a good practice. With so many moving pieces it is easy to write a test that passes accidentally because of some "bleed over" from another test. Sometimes it can complicate things like debugging though!

We could disable the random order, but I doubt that would help fix the underlying issue. And long-term I would prefer to keep the tests random.

@kenjis
Copy link
Member Author

kenjis commented May 23, 2022

I got the cause.

This test case shows the cause.

--- a/tests/Unit/PasswordsTest.php
+++ b/tests/Unit/PasswordsTest.php
@@ -6,6 +6,7 @@
 use CodeIgniter\Shield\Config\Auth as AuthConfig;
 use CodeIgniter\Shield\Entities\User;
 use CodeIgniter\Test\CIUnitTestCase;
+use Config\Services;
 
 /**
  * @internal
@@ -14,6 +15,9 @@ final class PasswordsTest extends CIUnitTestCase
 {
     public function testEmptyPassword()
     {
+        Services::reset();
+        d(Services::autoloader()->getNamespace());
+
         $passwords = new Passwords(new AuthConfig());
 
         $result = $passwords->check('', new User());
$ vendor/bin/phpunit tests/Unit/PasswordsTest.php b/tests/Unit/PasswordsTest.php
PHPUnit 9.5.20 #StandWithUkraine

Runtime:       PHP 8.0.18
Configuration: /Users/kenji/work/codeigniter/codeigniter-shield/phpunit.xml
Random Seed:   1653307776

F                                                                   1 / 1 (100%)
┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Services::autoloader()->getNamespace()                                                                                                         │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
array (0) []
══════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Called from <ROOT>/tests/Unit/PasswordsTest.php:19 [d()]


Time: 00:00.069, Memory: 12.00 MB

There was 1 failure:

1) Tests\Unit\PasswordsTest::testEmptyPassword
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'A Password is required.'
+'Auth.errorPasswordEmpty'

/Users/kenji/work/codeigniter/codeigniter-shield/tests/Unit/PasswordsTest.php:26

FAILURES!
Tests: 1, Assertions: 2, Failures: 1.

@kenjis
Copy link
Member Author

kenjis commented May 23, 2022

@MGatner The random order is fine. But it requires all test methods to be independent, or with proper @depends.
At least all devs should know we are using random.

Services::reset() behavior seems to be for rare cases. I propose to change the default value.
codeigniter4/CodeIgniter4#6020

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 a pull request may close this issue.

2 participants