Skip to content

Conversation

@MagnusSamuelsson
Copy link
Contributor

Fix for issue 115:

1) auth_userkey\core_userkey_manager_test::test_create_correct_key_if_iprestriction_is_string
Failed asserting that '1765964965' matches expected 1765964966.

/home/runner/work/moodle-auth_userkey/moodle-auth_userkey/moodle/auth/userkey/tests/core_userkey_manager_test.php:86
/home/runner/work/moodle-auth_userkey/moodle-auth_userkey/moodle/auth/userkey/tests/core_userkey_manager_test.php:121

By adding a fault tolerance of +/-1 to the test

@MagnusSamuelsson
Copy link
Contributor Author

Ah, shoot. It got an ugly extra merge commit.
Tell me if I should send a new PR.

@MagnusSamuelsson
Copy link
Contributor Author

I think I was a little bit early with the new attribute
#[CoversClass(\auth_userkey\core_userkey_manager::class)]

Got alot of warnings in moodle 405 tests complaining about that. I will remove it

@dmitriim
Copy link
Member

@MagnusSamuelsson it feels like you went a bit wrong direction fixing the issue.

  1. We don't need an extra parameter for clock in core_userkey_manager. We should just use \core\di::get(\core\clock::class)->time(); instead of php time().
  2. In tests we should call $this->mock_clock_with_frozen(); method before testing anything that could be time related. This will set frozen_clock intto DI container and then in core_userkey_manager we will be able to user frozen time for everything and could test against the frozen number.
  3. If you would like to use DI for core_userkey_manager, the you'd need to use it correctly. That means you should use di_configuration hook and use userkey_manager_interface to set core_userkey_manager into a DI container and then in tests you could override it by setting fake_userkey_manager into DI before testing. This will probably make set_userkey_manager method redundant for auth class as we could just pick userkey_manager_interface from DI container. I'd also rename userkey_manager_interface into userkey_manager so it looks better as DI id.

Saying all that I think 3 should be implemented as different PR (if you keen to continue) and in this PR we should focus on fixing #115

Hope that all makes sense.

Thoughts?

@MagnusSamuelsson
Copy link
Contributor Author

@dmitriim Ah, yes, you are right. I think i drifted away a little bit!
It makes sense!

@MagnusSamuelsson
Copy link
Contributor Author

@dmitriim So, simple as this?
I skipped the time() in auth_plugin_test, since it removes 3000 seconds from it, it should never be an issue.

Or what do you think?

@dmitriim dmitriim merged commit 84fcbb8 into catalyst:MOODLE_405_STABLE Dec 18, 2025
11 checks passed
@MagnusSamuelsson MagnusSamuelsson deleted the fix/test-time-issue branch December 18, 2025 22:46
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