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

chore: CI should have human readable logs + json for reporting #2241

Closed
wants to merge 4 commits into from

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Jan 29, 2025

Problem / Description

We recently switched to a JSON reporter for our tests.
This created a side-effect where logs were now being printed in JSON, instead of human-readable format (spec)

Solution

Use multi-reporter:

  • use spec (human readable) for logs (stdout)
  • use JSON for report

Checklist

  • Code changes are covered by unit tests.
  • Code changes are covered by e2e tests, if applicable.
  • Dogfooding has been performed, if feasible.
  • A test version has been published, if required.
  • All CI checks pass successfully.

Copy link

github-actions bot commented Jan 29, 2025

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 84.74 KB (0%) 1.7 s (0%) 11.3 s (-20.66% 🔽) 13 s
Waku Simple Light Node 135.73 KB (0%) 2.8 s (0%) 17 s (-6.26% 🔽) 19.7 s
ECIES encryption 22.73 KB (0%) 455 ms (0%) 5.4 s (+10.63% 🔺) 5.8 s
Symmetric encryption 22.23 KB (0%) 445 ms (0%) 4.4 s (-19.37% 🔽) 4.9 s
DNS discovery 70.8 KB (0%) 1.5 s (0%) 11.9 s (-5.18% 🔽) 13.3 s
Peer Exchange discovery 71.8 KB (0%) 1.5 s (0%) 8 s (-27.9% 🔽) 9.5 s
Local Peer Cache Discovery 65.39 KB (0%) 1.4 s (0%) 11.2 s (-20.73% 🔽) 12.5 s
Privacy preserving protocols 76.52 KB (0%) 1.6 s (0%) 11.7 s (-1.37% 🔽) 13.3 s
Waku Filter 78.11 KB (0%) 1.6 s (0%) 11.6 s (+9.99% 🔺) 13.2 s
Waku LightPush 75.6 KB (0%) 1.6 s (0%) 13.5 s (+15.08% 🔺) 15 s
History retrieval protocols 75.78 KB (0%) 1.6 s (0%) 11.7 s (+3.78% 🔺) 13.2 s
Deterministic Message Hashing 7.32 KB (0%) 147 ms (0%) 2.1 s (-0.74% 🔽) 2.2 s

@danisharora099 danisharora099 force-pushed the chore/human-readable-ci branch from 289fc62 to a528f27 Compare January 29, 2025 16:15
@danisharora099 danisharora099 marked this pull request as ready for review January 29, 2025 16:24
@danisharora099 danisharora099 requested a review from a team as a code owner January 29, 2025 16:24
@@ -47,14 +49,17 @@ jobs:

- run: npm run build:esm

- name: Create reports directory
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we create and save reports? Is it to be displayed in the pipeline later?
I know before we were doing so to push allure reports but why this one is done?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

similar situation, we generate the reports and use GH UI to display it

Copy link
Collaborator

@weboko weboko left a comment

Choose a reason for hiding this comment

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

seems good but test reporting has failed

@danisharora099 danisharora099 force-pushed the chore/human-readable-ci branch from a528f27 to 76ec0d5 Compare January 31, 2025 11:02
@danisharora099
Copy link
Collaborator Author

Moving to draft for now based on:

There's a bigger problem with using this test reporter:

This reporter expects JSON reporting; this leads us to generate reports in both spec/human-readable (for CI logs) and JSON (for the above test reporter)
- `mocha-multi-reporters` has the ability to generate reports in multiple formats, but that leaves us with JSON reports also being logged in the CI, along with spec

There is no way to disable logging for one reporter through Mocha.

So:
- either we run our tests twice, once through JSON and once through spec (not ideal)
- implement a custom reporter
- use some other reporter/testing framework


Are we considering going back to Allure? This will help with making a decision on how to best move forward.

If no, we can explore implementing a custom reporter, or infact switching to something like Jest or Vitest. They seem to handle reporting better and could give us the flexibility we need for both readable output and JSON reports without the current conflicts

https://discord.com/channels/1110799176264056863/1333438262013989018/1334858240235929632

@danisharora099 danisharora099 marked this pull request as draft January 31, 2025 12:10
@danisharora099
Copy link
Collaborator Author

redundant based on allure merge back

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