Skip to content

infra(ci): run against playground#3095

Closed
mshima wants to merge 28 commits into
faker-js:nextfrom
mshima:patch-2
Closed

infra(ci): run against playground#3095
mshima wants to merge 28 commits into
faker-js:nextfrom
mshima:patch-2

Conversation

@mshima

@mshima mshima commented Sep 7, 2024

Copy link
Copy Markdown
Contributor

Main types index.d.ts doesn’t exist.
Fix types to match '.' exports.

ref: jhipster/generator-jhipster#27179

@mshima mshima requested a review from a team as a code owner September 7, 2024 14:13
@netlify

netlify Bot commented Sep 7, 2024

Copy link
Copy Markdown

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 356e3ae
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/66e03332d2c0220008ede2e5
😎 Deploy Preview https://deploy-preview-3095.fakerjs.dev
📱 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.

@mshima mshima changed the title chore: fix main types fix: fix main types Sep 7, 2024
@codecov

codecov Bot commented Sep 7, 2024

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.97%. Comparing base (18ab2c7) to head (356e3ae).
⚠️ Report is 421 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3095      +/-   ##
==========================================
+ Coverage   99.96%   99.97%   +0.01%     
==========================================
  Files        2776     2776              
  Lines      226260   226260              
  Branches      945      591     -354     
==========================================
+ Hits       226183   226207      +24     
+ Misses         77       53      -24     

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Comment thread test/faker.spec.ts
@Shinigami92 Shinigami92 added the s: invalid This doesn't seem right label Sep 7, 2024
@mshima

mshima commented Sep 7, 2024

Copy link
Copy Markdown
Contributor Author

Same fix is applied in #3094.
Alternative fix is applied in #3093 (removes types from root)

The above PRs tries to fix CommonJS types too, while this fixes esm types.

@Shinigami92

Copy link
Copy Markdown
Member

you can checkout https://github.com/faker-js/playground and test your changes

@mshima

mshima commented Sep 7, 2024

Copy link
Copy Markdown
Contributor Author

you can checkout https://github.com/faker-js/playground and test your changes

Why don’t integrate playground on ci tests?

@Shinigami92

Copy link
Copy Markdown
Member

you can checkout faker-js/playground and test your changes

Why don’t integrate playground on ci tests?

PR welcome, go and try for it

@mshima mshima marked this pull request as draft September 7, 2024 17:45
@Shinigami92

Copy link
Copy Markdown
Member

@mshima thanks, this is a huge step forward and I wished for something like this a long time ❤️
you need to rebind/relink the repo faker to the playground test run
so something like modifying pnpm override before installing the playground deps

@mshima mshima marked this pull request as ready for review September 7, 2024 17:55
@mshima

mshima commented Sep 7, 2024

Copy link
Copy Markdown
Contributor Author

@Shinigami92 I am on mobile now, so I cannot rebase.
I think it’s only missing steps names now.
Feel free to modify the PR.

Playground is passing 🚀

@mshima

This comment was marked as outdated.

Comment thread package.json Outdated
@mshima

mshima commented Sep 10, 2024

Copy link
Copy Markdown
Contributor Author

IMO running playground in CI:

We could do that but it has a side effect, that all features that are used in the playground that might experience breaking changes need to be updated to the new feature before it is available as a dependency. This is also the case for the releasenPR but that those are even rarer.

  • the workflow can be ignored if it's not required to pass.
  • contributor will be warned about breaking changes or unwanted breaking changes.

Moving playground to the main repository can be considered too.

@Shinigami92

Copy link
Copy Markdown
Member
  • the workflow can be ignored if it's not required to pass.
  • contributor will be warned about breaking changes or unwanted breaking changes.

I agree 👍 we can mark the CI check as "not required" and if it breaks something, we are just "informed"

@mshima mshima dismissed stale reviews from prisis and Shinigami92 via 97237d9 September 10, 2024 11:47
Shinigami92
Shinigami92 previously approved these changes Sep 10, 2024
@ST-DDT ST-DDT added p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision labels Sep 12, 2024
@ST-DDT

ST-DDT commented Sep 12, 2024

Copy link
Copy Markdown
Member

I agree 👍 we can mark the CI check as "not required" and if it breaks something, we are just "informed"

Please note that that will only work once and will cause confusion for future PRs (that will fail the CI even if they didn't break anything because next is already "broken").

I would rather move the playgrounds inside the main repo to avoid these bidirectional dependencies.

@ST-DDT ST-DDT added this to the vAnytime milestone Sep 12, 2024
@ST-DDT

ST-DDT commented Sep 12, 2024

Copy link
Copy Markdown
Member

Team Decision

We don't want to add bi-directional dependencies between the playground and the faker repo.
We think that moving the playground to the main repo is the best solution for that, but that would require extensive work e.g. converting the repo into a mono-repo, what we don't want to do yet.

We would like to still run the pipeline on demand somehow e.g. using a comment, but we are currently unsure about the exact expectation we have for the pipeline. We will talk about this again in a future team meeting.

@ST-DDT

ST-DDT commented Sep 12, 2024

Copy link
Copy Markdown
Member

If you have an idea, on how to move the playground to the main repo feel free to open a PR with a suggestion.

@ST-DDT ST-DDT added needs rebase There is a merge conflict s: on hold Blocked by something or frozen to avoid conflicts and removed s: needs decision Needs team/maintainer decision labels Sep 14, 2024
@ST-DDT

ST-DDT commented Sep 19, 2024

Copy link
Copy Markdown
Member

We are looking into restructuring our project setup to solve this issue. For now, you don't have to update this PR.
Thanks for bringing this to our attention.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Dec 7, 2024
@xDivisionByZerox

Copy link
Copy Markdown
Member

I'm closing this, since we introduced the ability to automatically test our Faker against the Faker playground in #2988 as part of the restructuring of our automatic release process.

@xDivisionByZerox xDivisionByZerox removed the s: needs decision Needs team/maintainer decision label Dec 3, 2025
@ST-DDT ST-DDT removed this from the vAnytime milestone Jun 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: infra Changes to our infrastructure or project setup needs rebase There is a merge conflict p: 1-normal Nothing urgent s: on hold Blocked by something or frozen to avoid conflicts

5 participants