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

First stab at handling console messages while a test runs #35

Merged
merged 5 commits into from
Jun 3, 2024

Conversation

DmitrySharabin
Copy link
Collaborator

@DmitrySharabin DmitrySharabin commented Jun 1, 2024

In this PR, I tried to implement @LeaVerou's suggestion on handling console messages from tests, including (as an addition to the main algorithm):

  • Refactor interceptConsole() so that it accepts a function as an argument
  • Add utility pluralize() method to correctly print out “message”/“messages” to the console

Here is what we have as a starting point:

Interactive.CLI.Console.messages.mp4

Let's improve it and make it more awesome together!

- Refactor `interceptConsole()` so that it accepts a function as an argument

- Add utility `pluralize()` method to correctly print out “message”/“messages” to the console
Copy link

netlify bot commented Jun 1, 2024

Deploy Preview for h-test ready!

Name Link
🔨 Latest commit 5139870
🔍 Latest deploy log https://app.netlify.com/sites/h-test/deploys/665e08774510890008e1ed47
😎 Deploy Preview https://deploy-preview-35--h-test.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/env/node.js Outdated Show resolved Hide resolved
Copy link
Owner

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

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

Code seems ok ("console message" is a bit of an odd term, but not sure what else we could use. "Console output" is more fitting but not countable, so we can't say 3 console outputs.). Not a fan of the multiple sequential parens. I don't think the console messages need parens, a separator is probably fine, or even a comma.

@DmitrySharabin
Copy link
Collaborator Author

"console message" is a bit of an odd term, but not sure what else we could use. "Console output" is more fitting but not countable, so we can't say 3 console outputs.

How about “N output messages,” “N log messages,” or simply “N messages,” then?

Not a fan of the multiple sequential parens.

Me neither.

What if we move info about console messages before the time spent? It'll become aligned with the summary we provide for groups.

Like so:

image

Co-authored-by: Lea Verou <[email protected]>
@LeaVerou
Copy link
Owner

LeaVerou commented Jun 3, 2024

"console message" is a bit of an odd term, but not sure what else we could use. "Console output" is more fitting but not countable, so we can't say 3 console outputs.

How about “N output messages,” “N log messages,” or simply “N messages,” then?

Not a fan of "log messages", since they may not all be log messages. Anyhow, that’s not a blocker, we can use whatever for now and change it very easily.

Not a fan of the multiple sequential parens.

Me neither.

What if we move info about console messages before the time spent? It'll become aligned with the summary we provide for groups.

Like so:

image

Much better!

@DmitrySharabin
Copy link
Collaborator Author

Not a fan of "log messages", since they may not all be log messages. Anyhow, that’s not a blocker, we can use whatever for now and change it very easily.

I went with “messages.” Yeah, we can change it anytime.

It's ready for your other review, then.

@DmitrySharabin DmitrySharabin merged commit 75d0305 into main Jun 3, 2024
4 checks passed
@DmitrySharabin DmitrySharabin deleted the first-stab-at-console-messages branch June 3, 2024 20:16
DmitrySharabin added a commit that referenced this pull request Jun 3, 2024
…t runs (#35)

* Refactor `interceptConsole()` so that it accepts a function as an argument

* Add utility `pluralize()` method to correctly print out “message”/“messages” to the console

* Move messages stats before time taken stats

* Rename: “console message(s)” → “message(s)”

We can change it later if we like for something else with ease.

---------

Co-Authored-By: Lea Verou <[email protected]>
DmitrySharabin added a commit that referenced this pull request Jun 3, 2024
…t runs (#35)

* Refactor `interceptConsole()` so that it accepts a function as an argument

* Add utility `pluralize()` method to correctly print out “message”/“messages” to the console

* Move messages stats before time taken stats

* Rename: “console message(s)” → “message(s)”

We can change it later if we like for something else with ease.

---------

Co-Authored-By: Lea Verou <[email protected]>
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