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

feat(blog): node.js test migration #993

Merged
merged 12 commits into from
Mar 25, 2024
Merged

Conversation

ematipico
Copy link
Member

No description provided.

Copy link

vercel bot commented Feb 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
astro-build ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 25, 2024 4:58pm

Copy link
Member

@HiDeoo HiDeoo left a comment

Choose a reason for hiding this comment

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

Left a few comments for possible typos I found while reading but I really enjoyed the retrospective analysis 🌟

src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

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

Amazing post @ematipico, I left a couple of suggestions. Let me know what you think 😁

src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Good work, I've left my suggestions for you to consider 🙌

src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
Copy link
Member

@dreyfus92 dreyfus92 left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@@ -0,0 +1,129 @@
---
title: "Migration to Node.js test runner: a retrospective"
Copy link
Member

Choose a reason for hiding this comment

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

Can I make a title pitch:

  • "Migrating 500 tests from Mocha to node:test"
  • "Migrating 500 tests from Mocha to Node.js"

Either of these -- or something in a similar vein -- sound significantly more interesting if you imagine someone scrolling on Twitter, Hacker News, etc. which would ultimately lead to more readers.

Only thing I can't think of is a better name for the "Node.js Test Runner" but I think "to Node.js" captures it while leaving a little room for intrigue (wait, what do they mean "to Node.js").

Copy link
Contributor

Choose a reason for hiding this comment

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

Is Node.js not confusing to you? Mocha runs on Node.js too. I like using node:test though

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

OK! This was great @ematipico , and this might look like a lot but almost all of these are very tiny grammar fixes. A few are some suggested rewordings, so see what you think!

src/content/blog/node-test-migration.mdx Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Looks like Sarah is making some great suggestions around the wording, but otherwise for me (before I sign off), the content/topic looks great to me 👍

Co-authored-by: Sarah Rainsberger <[email protected]>
---
title: "Migrating 500+ tests from Mocha to Node.js"
description: "A retrospective of how Astro migrated more than 500 test suites from Mocha to Node.js test runner."
publishDate: "March 21, 2024"
Copy link
Member

Choose a reason for hiding this comment

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

Reminder to update to the date we actually publish!

Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

Final pass!

src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
src/content/blog/node-test-migration.mdx Outdated Show resolved Hide resolved
@sarah11918
Copy link
Member

All of Yan's corrections are correct and should be committed!

@ematipico ematipico merged commit 3633b6b into main Mar 25, 2024
3 checks passed
@ematipico ematipico deleted the chore/test-migration-article branch March 25, 2024 16:58
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.

8 participants