feat: Check logs against parts of the message only by paulbalandan · Pull Request #6704 · codeigniter4/CodeIgniter4

paulbalandan · 2022-10-17T10:27:19Z

Description
Currently, CIUnitTestCase::assertLogged() matches the logged message verbatim. For simple messages this is no problem but for complex, hard-to-build, or volatile messages, it may be hard to come up with the exact message. This PR adds the capability for the assertLogged method to compare only parts of the message instead of the whole.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-17T11:36:44Z

The implementation looks good.

kenjis · 2022-10-18T00:36:18Z

Something wrong with GA checks.

Some checks were not successful
1 failing and 5 successful checks

MGatner

I would lean towards creating a new assertLoggedStrict() or the like, but if you two prefer this I'm good with it.

Edit: the opposite, I suppose! assertLogContains()

paulbalandan · 2022-10-18T11:01:00Z

I was actually contemplating between a new method and modified method. I'll check what I can do.

kenjis · 2022-10-18T11:36:41Z

Generally It is better to avoid boolean flag parameter.
I prefer a new method, because it is easier to read.

$this->assertLogged('error', 'variable did not', false); // What's false? Jump to the method.$this->assertLogContains('error', 'variable did not');

kenjis · 2022-10-19T01:10:09Z

- *
- * @throws Exception
 */
 public function assertLogged(string $level, $expectedMessage = null)


This is not related to this PR, but why $expectedMessage can be null?

I'm not sure either.

🤦‍♂️

kenjis · 2022-10-19T01:10:49Z

+ /**
+ * Asserts that there is a log record that contains `$logMessage` in the message.
+ */
+ public function assertLogContains(string $level, string $logMessage, string $message = ''): void


$expectedMessage is better? assertLogged() uses it.

My understanding is $expectedMessage is used since the assertion compares the message as expected in the logs.
For assertLogContains, I think it is counter-intuitive to use $expectedMessage when it can only be a part of the whole expected message. I was also thinking whether to use $needle or $needleMessage as alternative name.

No preference from me

I am a little uncomfortable with both $needle and $needleMessage.
needle needs haystack.
So let's leave it as it is.

paulbalandan · 2022-10-17T10:27:19Z

Description
Currently, CIUnitTestCase::assertLogged() matches the logged message verbatim. For simple messages this is no problem but for complex, hard-to-build, or volatile messages, it may be hard to come up with the exact message. This PR adds the capability for the assertLogged method to compare only parts of the message instead of the whole.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-17T11:36:44Z

The implementation looks good.

kenjis · 2022-10-18T00:36:18Z

Something wrong with GA checks.

Some checks were not successful
1 failing and 5 successful checks

MGatner

I would lean towards creating a new assertLoggedStrict() or the like, but if you two prefer this I'm good with it.

Edit: the opposite, I suppose! assertLogContains()

paulbalandan · 2022-10-18T11:01:00Z

I was actually contemplating between a new method and modified method. I'll check what I can do.

kenjis · 2022-10-18T11:36:41Z

Generally It is better to avoid boolean flag parameter.
I prefer a new method, because it is easier to read.

$this->assertLogged('error', 'variable did not', false); // What's false? Jump to the method.$this->assertLogContains('error', 'variable did not');

kenjis · 2022-10-19T01:10:09Z

- *
- * @throws Exception
 */
 public function assertLogged(string $level, $expectedMessage = null)


This is not related to this PR, but why $expectedMessage can be null?

I'm not sure either.

🤦‍♂️

kenjis · 2022-10-19T01:10:49Z

+ /**
+ * Asserts that there is a log record that contains `$logMessage` in the message.
+ */
+ public function assertLogContains(string $level, string $logMessage, string $message = ''): void


$expectedMessage is better? assertLogged() uses it.

My understanding is $expectedMessage is used since the assertion compares the message as expected in the logs.
For assertLogContains, I think it is counter-intuitive to use $expectedMessage when it can only be a part of the whole expected message. I was also thinking whether to use $needle or $needleMessage as alternative name.

No preference from me

I am a little uncomfortable with both $needle and $needleMessage.
needle needs haystack.
So let's leave it as it is.

paulbalandan · 2022-10-17T10:27:19Z

Description
Currently, CIUnitTestCase::assertLogged() matches the logged message verbatim. For simple messages this is no problem but for complex, hard-to-build, or volatile messages, it may be hard to come up with the exact message. This PR adds the capability for the assertLogged method to compare only parts of the message instead of the whole.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-17T11:36:44Z

The implementation looks good.

kenjis · 2022-10-18T00:36:18Z

Something wrong with GA checks.

Some checks were not successful
1 failing and 5 successful checks

MGatner

I would lean towards creating a new assertLoggedStrict() or the like, but if you two prefer this I'm good with it.

Edit: the opposite, I suppose! assertLogContains()

paulbalandan · 2022-10-18T11:01:00Z

I was actually contemplating between a new method and modified method. I'll check what I can do.

kenjis · 2022-10-18T11:36:41Z

Generally It is better to avoid boolean flag parameter.
I prefer a new method, because it is easier to read.

$this->assertLogged('error', 'variable did not', false); // What's false? Jump to the method.$this->assertLogContains('error', 'variable did not');

kenjis · 2022-10-19T01:10:09Z

- *
- * @throws Exception
 */
 public function assertLogged(string $level, $expectedMessage = null)


This is not related to this PR, but why $expectedMessage can be null?

I'm not sure either.

🤦‍♂️

kenjis · 2022-10-19T01:10:49Z

+ /**
+ * Asserts that there is a log record that contains `$logMessage` in the message.
+ */
+ public function assertLogContains(string $level, string $logMessage, string $message = ''): void


$expectedMessage is better? assertLogged() uses it.

My understanding is $expectedMessage is used since the assertion compares the message as expected in the logs.
For assertLogContains, I think it is counter-intuitive to use $expectedMessage when it can only be a part of the whole expected message. I was also thinking whether to use $needle or $needleMessage as alternative name.

No preference from me

I am a little uncomfortable with both $needle and $needleMessage.
needle needs haystack.
So let's leave it as it is.

paulbalandan · 2022-10-17T10:27:19Z

Description
Currently, CIUnitTestCase::assertLogged() matches the logged message verbatim. For simple messages this is no problem but for complex, hard-to-build, or volatile messages, it may be hard to come up with the exact message. This PR adds the capability for the assertLogged method to compare only parts of the message instead of the whole.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-17T11:36:44Z

The implementation looks good.

kenjis · 2022-10-18T00:36:18Z

Something wrong with GA checks.

Some checks were not successful
1 failing and 5 successful checks

MGatner

I would lean towards creating a new assertLoggedStrict() or the like, but if you two prefer this I'm good with it.

Edit: the opposite, I suppose! assertLogContains()

paulbalandan · 2022-10-18T11:01:00Z

I was actually contemplating between a new method and modified method. I'll check what I can do.

kenjis · 2022-10-18T11:36:41Z

Generally It is better to avoid boolean flag parameter.
I prefer a new method, because it is easier to read.

$this->assertLogged('error', 'variable did not', false); // What's false? Jump to the method.$this->assertLogContains('error', 'variable did not');

kenjis · 2022-10-19T01:10:09Z

- *
- * @throws Exception
 */
 public function assertLogged(string $level, $expectedMessage = null)


This is not related to this PR, but why $expectedMessage can be null?

I'm not sure either.

🤦‍♂️

kenjis · 2022-10-19T01:10:49Z

+ /**
+ * Asserts that there is a log record that contains `$logMessage` in the message.
+ */
+ public function assertLogContains(string $level, string $logMessage, string $message = ''): void


$expectedMessage is better? assertLogged() uses it.

My understanding is $expectedMessage is used since the assertion compares the message as expected in the logs.
For assertLogContains, I think it is counter-intuitive to use $expectedMessage when it can only be a part of the whole expected message. I was also thinking whether to use $needle or $needleMessage as alternative name.

No preference from me

I am a little uncomfortable with both $needle and $needleMessage.
needle needs haystack.
So let's leave it as it is.

paulbalandan · 2022-10-17T10:27:19Z

Description
Currently, CIUnitTestCase::assertLogged() matches the logged message verbatim. For simple messages this is no problem but for complex, hard-to-build, or volatile messages, it may be hard to come up with the exact message. This PR adds the capability for the assertLogged method to compare only parts of the message instead of the whole.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-17T11:36:44Z

The implementation looks good.

kenjis · 2022-10-18T00:36:18Z

Something wrong with GA checks.

Some checks were not successful
1 failing and 5 successful checks

MGatner

I would lean towards creating a new assertLoggedStrict() or the like, but if you two prefer this I'm good with it.

Edit: the opposite, I suppose! assertLogContains()

paulbalandan · 2022-10-18T11:01:00Z

I was actually contemplating between a new method and modified method. I'll check what I can do.

kenjis · 2022-10-18T11:36:41Z

Generally It is better to avoid boolean flag parameter.
I prefer a new method, because it is easier to read.

$this->assertLogged('error', 'variable did not', false); // What's false? Jump to the method.$this->assertLogContains('error', 'variable did not');

kenjis · 2022-10-19T01:10:09Z

- *
- * @throws Exception
 */
 public function assertLogged(string $level, $expectedMessage = null)


This is not related to this PR, but why $expectedMessage can be null?

I'm not sure either.

🤦‍♂️

kenjis · 2022-10-19T01:10:49Z

+ /**
+ * Asserts that there is a log record that contains `$logMessage` in the message.
+ */
+ public function assertLogContains(string $level, string $logMessage, string $message = ''): void


$expectedMessage is better? assertLogged() uses it.

My understanding is $expectedMessage is used since the assertion compares the message as expected in the logs.
For assertLogContains, I think it is counter-intuitive to use $expectedMessage when it can only be a part of the whole expected message. I was also thinking whether to use $needle or $needleMessage as alternative name.

No preference from me

I am a little uncomfortable with both $needle and $needleMessage.
needle needs haystack.
So let's leave it as it is.

paulbalandan · 2022-10-17T10:27:19Z

Description
Currently, CIUnitTestCase::assertLogged() matches the logged message verbatim. For simple messages this is no problem but for complex, hard-to-build, or volatile messages, it may be hard to come up with the exact message. This PR adds the capability for the assertLogged method to compare only parts of the message instead of the whole.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-17T11:36:44Z

The implementation looks good.

kenjis · 2022-10-18T00:36:18Z

Something wrong with GA checks.

Some checks were not successful
1 failing and 5 successful checks

MGatner

I would lean towards creating a new assertLoggedStrict() or the like, but if you two prefer this I'm good with it.

Edit: the opposite, I suppose! assertLogContains()

paulbalandan · 2022-10-18T11:01:00Z

I was actually contemplating between a new method and modified method. I'll check what I can do.

kenjis · 2022-10-18T11:36:41Z

Generally It is better to avoid boolean flag parameter.
I prefer a new method, because it is easier to read.

$this->assertLogged('error', 'variable did not', false); // What's false? Jump to the method.$this->assertLogContains('error', 'variable did not');

kenjis · 2022-10-19T01:10:09Z

- *
- * @throws Exception
 */
 public function assertLogged(string $level, $expectedMessage = null)


This is not related to this PR, but why $expectedMessage can be null?

I'm not sure either.

🤦‍♂️

kenjis · 2022-10-19T01:10:49Z

+ /**
+ * Asserts that there is a log record that contains `$logMessage` in the message.
+ */
+ public function assertLogContains(string $level, string $logMessage, string $message = ''): void


$expectedMessage is better? assertLogged() uses it.

My understanding is $expectedMessage is used since the assertion compares the message as expected in the logs.
For assertLogContains, I think it is counter-intuitive to use $expectedMessage when it can only be a part of the whole expected message. I was also thinking whether to use $needle or $needleMessage as alternative name.

No preference from me

I am a little uncomfortable with both $needle and $needleMessage.
needle needs haystack.
So let's leave it as it is.

paulbalandan · 2022-10-17T10:27:19Z

Description
Currently, CIUnitTestCase::assertLogged() matches the logged message verbatim. For simple messages this is no problem but for complex, hard-to-build, or volatile messages, it may be hard to come up with the exact message. This PR adds the capability for the assertLogged method to compare only parts of the message instead of the whole.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-17T11:36:44Z

The implementation looks good.

kenjis · 2022-10-18T00:36:18Z

Something wrong with GA checks.

Some checks were not successful
1 failing and 5 successful checks

MGatner

I would lean towards creating a new assertLoggedStrict() or the like, but if you two prefer this I'm good with it.

Edit: the opposite, I suppose! assertLogContains()

paulbalandan · 2022-10-18T11:01:00Z

I was actually contemplating between a new method and modified method. I'll check what I can do.

kenjis · 2022-10-18T11:36:41Z

Generally It is better to avoid boolean flag parameter.
I prefer a new method, because it is easier to read.

$this->assertLogged('error', 'variable did not', false); // What's false? Jump to the method.$this->assertLogContains('error', 'variable did not');

kenjis · 2022-10-19T01:10:09Z

- *
- * @throws Exception
 */
 public function assertLogged(string $level, $expectedMessage = null)


This is not related to this PR, but why $expectedMessage can be null?

I'm not sure either.

🤦‍♂️

kenjis · 2022-10-19T01:10:49Z

+ /**
+ * Asserts that there is a log record that contains `$logMessage` in the message.
+ */
+ public function assertLogContains(string $level, string $logMessage, string $message = ''): void


$expectedMessage is better? assertLogged() uses it.

My understanding is $expectedMessage is used since the assertion compares the message as expected in the logs.
For assertLogContains, I think it is counter-intuitive to use $expectedMessage when it can only be a part of the whole expected message. I was also thinking whether to use $needle or $needleMessage as alternative name.

No preference from me

I am a little uncomfortable with both $needle and $needleMessage.
needle needs haystack.
So let's leave it as it is.

paulbalandan · 2022-10-17T10:27:19Z

Description
Currently, CIUnitTestCase::assertLogged() matches the logged message verbatim. For simple messages this is no problem but for complex, hard-to-build, or volatile messages, it may be hard to come up with the exact message. This PR adds the capability for the assertLogged method to compare only parts of the message instead of the whole.

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-17T11:36:44Z

The implementation looks good.

kenjis · 2022-10-18T00:36:18Z

Something wrong with GA checks.

Some checks were not successful
1 failing and 5 successful checks

MGatner

I would lean towards creating a new assertLoggedStrict() or the like, but if you two prefer this I'm good with it.

Edit: the opposite, I suppose! assertLogContains()

paulbalandan · 2022-10-18T11:01:00Z

I was actually contemplating between a new method and modified method. I'll check what I can do.

kenjis · 2022-10-18T11:36:41Z

Generally It is better to avoid boolean flag parameter.
I prefer a new method, because it is easier to read.

$this->assertLogged('error', 'variable did not', false); // What's false? Jump to the method.$this->assertLogContains('error', 'variable did not');

kenjis · 2022-10-19T01:10:09Z

- *
- * @throws Exception
 */
 public function assertLogged(string $level, $expectedMessage = null)


This is not related to this PR, but why $expectedMessage can be null?

I'm not sure either.

🤦‍♂️

kenjis · 2022-10-19T01:10:49Z

+ /**
+ * Asserts that there is a log record that contains `$logMessage` in the message.
+ */
+ public function assertLogContains(string $level, string $logMessage, string $message = ''): void


$expectedMessage is better? assertLogged() uses it.

My understanding is $expectedMessage is used since the assertion compares the message as expected in the logs.
For assertLogContains, I think it is counter-intuitive to use $expectedMessage when it can only be a part of the whole expected message. I was also thinking whether to use $needle or $needleMessage as alternative name.

No preference from me

I am a little uncomfortable with both $needle and $needleMessage.
needle needs haystack.
So let's leave it as it is.

paulbalandan added docs needed Pull requests needing documentation write-ups and/or revisions. 4.3 labels Oct 17, 2022

paulbalandan mentioned this pull request Oct 17, 2022
feat: Opt-in logging of deprecations #6705
Merged
5 tasks

kenjis added the enhancement PRs that improve existing functionalities label Oct 17, 2022

paulbalandan removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Oct 17, 2022

kenjis approved these changes Oct 18, 2022
View reviewed changes

MGatner reviewed Oct 18, 2022
View reviewed changes

paulbalandan added 2 commits October 18, 2022 19:43

feat: Check logs against parts of the message only
2302dbc

docs: Add to user guide and changelog
4894788

paulbalandan force-pushed the assert-logged-approx branch from 5e85901 to 4894788Compare October 18, 2022 11:59

paulbalandan changed the title ~~feat: Add $useExactComparison option to assertLogged~~feat: Check logs against parts of the message onlyOct 18, 2022

paulbalandan requested review from MGatner and kenjis October 18, 2022 12:06

kenjis reviewed Oct 19, 2022
View reviewed changes

MGatner approved these changes Oct 19, 2022
View reviewed changes

kenjis approved these changes Oct 20, 2022
View reviewed changes

kenjis merged commit ee3e329 into codeigniter4:4.3Oct 20, 2022

paulbalandan deleted the assert-logged-approx branch October 20, 2022 05:00