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

add 1.2.2 to bun-version matrix at workflows #1643

Merged
merged 9 commits into from
Feb 19, 2025
Merged

Conversation

kravetsone
Copy link
Contributor

Description

Just add bun version from new major to CI matrix

Testing

CI

Checklist

  • Conducted a self-review of the code changes.
  • Updated documentation, if necessary.
  • Added tests to validate the functionality or fix.

@moltar moltar requested review from DarkGL and hoeck February 9, 2025 22:52
@hoeck
Copy link
Collaborator

hoeck commented Feb 10, 2025

Thanks @kravetsone

Could you please check whether that works with the existing benchmark frontend (having two minor bun versions):

const BUN_VERSIONS = [1];

As far as I am aware of, it is only able to fetch a single unique major version for each runtime. For Node that was was always sufficient.

No Idea how to fix this quickly though. Instead of docs/results/bun-1.json we'd need to create docs/results/bun-1_1.json and docs/results/bun-1_2.json files and fetch and bucket and display them accordingly in the frontend.

A quick workaround could be maybe to only benchmark bun 1.2?

@kravetsone
Copy link
Contributor Author

Thanks @kravetsone

Could you please check whether that works with the existing benchmark frontend (having two minor bun versions):

const BUN_VERSIONS = [1];

As far as I am aware of, it is only able to fetch a single unique major version for each runtime. For Node that was was always sufficient.

No Idea how to fix this quickly though. Instead of docs/results/bun-1.json we'd need to create docs/results/bun-1_1.json and docs/results/bun-1_2.json files and fetch and bucket and display them accordingly in the frontend.

A quick workaround could be maybe to only benchmark bun 1.2?

As hack we can probably use minor version as major for Bun here

const bunVersionRegex = /([0-9]+)\./;

Potentially can solve it (but I can't check now)

@DarkGL
Copy link
Collaborator

DarkGL commented Feb 10, 2025

I don't think we need to leave tests for 1.1.43

IMO bun is fast changing and still in heavy development process, and I think most of the user immediately update to the newest version, especially with how easy it is with bun upgrade

@DarkGL
Copy link
Collaborator

DarkGL commented Feb 13, 2025

@kravetsone could you change it?

@kravetsone
Copy link
Contributor Author

@kravetsone could you change it?

Okay!

@kravetsone
Copy link
Contributor Author

Can we check this logic?

I don't think we need to leave tests for 1.1.43

IMO bun is fast changing and still in heavy development process, and I think most of the user immediately update to the newest version, especially with how easy it is with bun upgrade

Sorry huh but i make workaround for bun to include Major.Minor in versioning
i guess why not?
More better way to check after minors ships (like a lot of JSC bumps and etc)

@@ -29,9 +29,23 @@ function pathFromStack() {
}
throw new Error('Could not get dirname');
}

function getRuntimeWithVersion() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implement this
because determine runtime by USER env is bad practic

Copy link
Contributor Author

@kravetsone kravetsone Feb 13, 2025

Choose a reason for hiding this comment

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

before this npm run start:bun will generate results for node@22 (but its Bun results)

@DarkGL
Copy link
Collaborator

DarkGL commented Feb 14, 2025

Linting errors

@kravetsone
Copy link
Contributor Author

oh sorry forgot about it

@kravetsone
Copy link
Contributor Author

idk how to run lint command here lol

its just return

 54:26  error  Delete `␍`  prettier/prettier
  55:23  error  Delete `␍`  prettier/prettier
  56:23  error  Delete `␍`  prettier/prettier
  57:27  error  Delete `␍`  prettier/prettier
  58:33  error  Delete `␍`  prettier/prettier
  59:33  error  Delete `␍`  prettier/prettier
  60:44  error  Delete `␍`  prettier/prettier
  61:41  error  Delete `␍`  prettier/prettier
  62:30  error  Delete `␍`  prettier/prettier
  63:1   error  Delete `␍`  prettier/prettier
  64:59  error  Delete `␍`  prettier/prettier
  65:10  error  Delete `␍`  prettier/prettier
  66:21  error  Delete `␍`  prettier/prettier
  67:48  error  Delete `␍`  prettier/prettier
  68:54  error  Delete `␍`  prettier/prettier
  69:9   error  Delete `␍`  prettier/prettier
  70:12  error  Delete `␍`  prettier/prettier
  71:24  error  Delete `␍`  prettier/prettier
  72:4   error  Delete `␍`  prettier/prettier
  73:1   error  Delete `␍`  prettier/prettier
  74:71  error  Delete `␍`  prettier/prettier
  75:32  error  Delete `␍`  prettier/prettier
  76:65  error  Delete `␍`  prettier/prettier
  77:6   error  Delete `␍`  prettier/prettier
  78:4   error  Delete `␍`  prettier/prettier

✖ 5341 problems (5341 errors, 0 warnings)
  5341 errors and 0 warnings potentially fixable with the `--fix` option.

@DarkGL
Copy link
Collaborator

DarkGL commented Feb 17, 2025

@kravetsone
Copy link
Contributor Author

It's because of line ending

https://stackoverflow.com/questions/1552749/difference-between-cr-lf-lf-and-cr-line-break-types

run lint:fix

Yeah i know about this
But why lint:fix change it to windows eol instead of existing?

Can anyone fix linter issue?

Sorry for this)

@kravetsone
Copy link
Contributor Author

It's because of line ending

https://stackoverflow.com/questions/1552749/difference-between-cr-lf-lf-and-cr-line-break-types

run lint:fix

Yeah i know about this
But why lint:fix change it to windows eol instead of existing?

Can anyone fix linter issue?

Sorry for this)

I can run lint:fix but it change every file in repo lol

@hoeck
Copy link
Collaborator

hoeck commented Feb 19, 2025

I can run lint:fix but it change every file in repo lol

mhh that's weird, have you tried running prettier? that should normalize newlines to \n. Or maybe there is a setting missing in your editor or git config (git does weird magical stuff with line endings too).

Can anyone fix linter issue?

Yeah if the above doesn't work I can fix it for you.

@kravetsone
Copy link
Contributor Author

I can run lint:fix but it change every file in repo lol

mhh that's weird, have you tried running prettier? that should normalize newlines to \n. Or maybe there is a setting missing in your editor or git config (git does weird magical stuff with line endings too).

Can anyone fix linter issue?

Yeah if the above doesn't work I can fix it for you.

can u approve CI again?

Looks like there not formatting error

but when i run prettier -w or eslint fixes

image

it just do that

boring tooling(

Copy link
Collaborator

@hoeck hoeck left a comment

Choose a reason for hiding this comment

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

Looks good now (no linting errors any more), albeit a bit hacky 😁. What do you think @DarkGL ?

@DarkGL
Copy link
Collaborator

DarkGL commented Feb 19, 2025

lgtm

@DarkGL DarkGL merged commit 9c28e1a into moltar:master Feb 19, 2025
10 checks passed
@kravetsone
Copy link
Contributor Author

image

for now 1.2 json data is 404

is it normal?

@kravetsone kravetsone mentioned this pull request Feb 19, 2025
3 tasks
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.

4 participants