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

test(log): new unit tests #7988

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

test(log): new unit tests #7988

wants to merge 10 commits into from

Conversation

velvetrevolver
Copy link
Contributor

Description

write unit tests for logCapture

XO-184

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

Copy link
Member

@julien-f julien-f left a comment

Choose a reason for hiding this comment

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

Too many of these tests are not related to captureLogs(), if you want to keep them, move them to another commit & file.

@xen-orchestra/log/capture.test.js Outdated Show resolved Hide resolved
@xen-orchestra/log/capture.test.js Outdated Show resolved Hide resolved
Comment on lines 50 to 53
assert.equal(logsTransportDefault[0]?.message, expected1.message)
assert.equal(typeof logsTransportDefault[0]?.level, 'number', `level attribute should exist`)
assert.ok(typeof logsTransportDefault[0]?.namespace === 'string', `namespace attribute should exist`)
assert.ok(logsTransportDefault[0]?.time instanceof Date, `time attribute should exist`)
Copy link
Member

Choose a reason for hiding this comment

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

Do not use optional chaining when you know the entry exist.

}
logger.debug(expected1.message)
assert.equal(logsTransportDefault[0]?.message, expected1.message)
assert.equal(typeof logsTransportDefault[0]?.level, 'number', `level attribute should exist`)
Copy link
Member

Choose a reason for hiding this comment

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

Do not bother adding an assert message unless it brings a new info.

In this case, I don't see how it would really help, if the test fails, we know which assert did and we fix it.

To be clear, it's absolutely not a problem that you provided a message, but you don't have to 🙂

logger.debug(expected1.message)
assert.equal(logsTransportDefault[0]?.message, expected1.message)
assert.equal(typeof logsTransportDefault[0]?.level, 'number', `level attribute should exist`)
assert.ok(typeof logsTransportDefault[0]?.namespace === 'string', `namespace attribute should exist`)
Copy link
Member

Choose a reason for hiding this comment

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

Use assert.equal instead because it will show the actual value which is important to understand the issue when the assert fails.

logsTransportFatal.length = 0
})

it('should log test with all its attributes', () => {
Copy link
Member

Choose a reason for hiding this comment

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

That's not what you are testing here: you are testing that logs not in captureLogs() are handled by the fallback transport.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested what appeared in the Usage.md, here it was the "producer" part presenting the logger.

assert.ok(typeof logsTransportDefault[0]?.namespace === 'string', `namespace attribute should exist`)
assert.ok(logsTransportDefault[0]?.time instanceof Date, `time attribute should exist`)
})
it('should log test 1 and 2', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what you are testing here and how it is related to captureLogs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested in order what was in the Usage.md, there it was the "producer" part presenting the logger.

@julien-f julien-f changed the title write unit tests for logCapture test(log): new unit tests Sep 20, 2024
Comment on lines 71 to 99
it('should not dedup logs', () => {
configure(
createCaptureTransport([
{
transport: transportTest,
},
])
)

for (let i = 0; i < 3; i++) {
logger.debug('this line should be logged 3 times')
}
assert.equal(logsTransportDefault.length, 3)
})
it('should dedup logs', () => {
configure(
createCaptureTransport([
dedupe({
timeout: 50, // without a defined timeout, the test will wait for 10 minutes
transport: transportTest,
}),
])
)

for (let i = 0; i < 3; i++) {
logger.debug('this line should be logged only once')
}
assert.equal(logsTransportDefault.length, 1)
})
Copy link
Member

Choose a reason for hiding this comment

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

Out of captureLogs() scope, you can move into its own file.

Comment on lines 41 to 46
const expected1 = {
level: 20,
namespace: 'test-logger',
message: 'test 1 with all its attributes',
time: new Date(),
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are you creating this object? you are not using it? 🤔

Copy link
Contributor Author

@velvetrevolver velvetrevolver Sep 24, 2024

Choose a reason for hiding this comment

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

The message is tested line 49 :
assert.equal(logsTransportDefault[0].message, expected1.message)

Copy link
Member

Choose a reason for hiding this comment

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

Yes but you don't need this object, you can simply create a message variable if that's all you need.

assert.equal(typeof logsTransportDefault[0].namespace, 'string', `namespace attribute should exist`)
assert.equal(logsTransportDefault[0].time instanceof Date, true, 'time attribute should exist')
})
it('should log test 1 and 2', () => {
Copy link
Member

Choose a reason for hiding this comment

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

What feature does this case test that the the previous one did not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It test the "test transport" can contain many logs, and not only one constantly overwriten. Needed by the captureLog test after when we test multiple logs.

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.

3 participants