-
Notifications
You must be signed in to change notification settings - Fork 404
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
add test for test-logging-tx-observer #1630
add test for test-logging-tx-observer #1630
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, needs a few changes here. It also breaks the build (see the build details in the PR and github actions). Please run the unit and integration tests before submitting the PR to avoid this
packages/caliper-core/lib/worker/tx-observers/tx-observer-dispatch.js
Outdated
Show resolved
Hide resolved
packages/caliper-core/test/worker/tx-observers/logging-tx-observer.js
Outdated
Show resolved
Hide resolved
packages/caliper-core/test/worker/tx-observers/tx-observer-dispatch.js
Outdated
Show resolved
Hide resolved
packages/caliper-core/test/worker/tx-observers/logging-tx-observer.js
Outdated
Show resolved
Hide resolved
a490812
to
289b878
Compare
Signed-off-by: Babatunde Sanusi <[email protected]>
df57bac
to
02b94a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last change
}); | ||
|
||
describe('On initialization', () => { | ||
it('should set up the logger with default settings if no options are provided', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to change this test, logStubs.info will always exist because you create it in the test. So the test doesn't actually prove anything
true, would remove it now
…On Thu, 10 Oct 2024 at 08:38, Dave Kelsey ***@***.***> wrote:
***@***.**** requested changes on this pull request.
One last change
------------------------------
In packages/caliper-core/test/worker/tx-observers/logging-tx-observer.js
<#1630 (comment)>:
> + logStub = CaliperUtils.getLogger().info;
+ observer = createLoggingTxObserver({ messageLevel: 'info' }, null, 0);
+ });
+
+ afterEach(() => {
+ sinon.restore();
+ logStub.resetHistory();
+ });
+
+ after(() => {
+ mockery.deregisterAll();
+ mockery.disable();
+ });
+
+ describe('On initialization', () => {
+ it('should set up the logger with default settings if no options are provided', () => {
You need to change this test, logStubs.info will always exist because you
create it in the test. So the test doesn't actually prove anything
—
Reply to this email directly, view it on GitHub
<#1630 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH6TKS2NBGHBEFK76BB3YXDZ2YVHXAVCNFSM6AAAAABOAQ4Z5WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNJVGEZTKOBVGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Signed-off-by: Babatunde Sanusi <[email protected]>
02b94a9
to
ca50352
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Checklist
Issue/User story
Partially addresses #1606
Steps to Reproduce
Existing issues
Design of the fix
Validation of the fix
After this PR: Running the tests in caliper-core the listed %stmts in the code coverage report should tally with the below listed
Before the PR: Running the tests in caliper-core the listed %stmts in the code coverage report the below listed was what was there before
Automated Tests
What documentation has been provided for this pull request