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

test(format/grit): add tests for grit formatter #3937

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

branberry
Copy link
Contributor

@branberry branberry commented Sep 16, 2024

Summary

Add basic testing setup for the Grit formatter. Also added some additional configuration to handle Grit files.

Test Plan

Verify that tests for Grit execute successfully

@github-actions github-actions bot added A-Project Area: project A-Formatter Area: formatter labels Sep 16, 2024
Copy link

codspeed-hq bot commented Sep 16, 2024

CodSpeed Performance Report

Merging #3937 will degrade performances by 7.05%

Comparing branberry:feat/grit-formatter-tests (b1bcdd7) with main (33d0958)

Summary

❌ 1 regressions
✅ 106 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main branberry:feat/grit-formatter-tests Change
dojo_11880045762646467684.js[cached] 7.9 ms 8.5 ms -7.05%

@github-actions github-actions bot added the A-Parser Area: parser label Sep 17, 2024
slotmap = { workspace = true, features = ["serde"] }
tracing = { workspace = true, features = ["attributes", "log"] }
biome_grit_syntax = { workspace = true }

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the formatting changes might have been due to the newline here.


#[derive(Debug, Default, PartialEq, Eq)]

pub(crate) struct GritFileHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we're adding a file handler for Grit 👍

Copy link
Contributor Author

@branberry branberry Sep 19, 2024

Choose a reason for hiding this comment

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

Glad to hear! Haha. I've been following some of the other examples, and I've seen that we typically create a file handler struct as a part of the testing process.

Not sure if I'm doing too much work here, but it sounds like I'm working in the right direction!

As a side note, I'm a Rust newbie so any pointers on how I can improve my Rust code is welcome and appreciated :D

@branberry branberry changed the title (DRAFT) feat(formatter/grit): add tests feat(formatter/grit): add tests for grit formatter Sep 19, 2024
@branberry branberry changed the title feat(formatter/grit): add tests for grit formatter test(formatter/grit): add tests for grit formatter Sep 19, 2024
@branberry branberry changed the title test(formatter/grit): add tests for grit formatter test(format/grit): add tests for grit formatter Sep 19, 2024
@branberry branberry marked this pull request as ready for review September 19, 2024 13:35
crates/biome_service/src/file_handlers/mod.rs Outdated Show resolved Hide resolved
let root_path = Path::new(concat!(env!("CARGO_MANIFEST_DIR"), "/tests/specs/"));

let Some(test_file) = SpecTestFile::try_from_file(spec_input_file, root_path, None) else {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend panicking in this else branch, otherwise the test would fail silently. This happened in the html formatter recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like that uncovered a bug! Working to address it. Thanks!

@github-actions github-actions bot added the L-HTML Language: HTML label Sep 19, 2024
@dyc3 dyc3 removed the L-HTML Language: HTML label Sep 19, 2024
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Nice!

@arendjr
Copy link
Contributor

arendjr commented Sep 19, 2024

Note there does seem to be a test failure still.

@branberry
Copy link
Contributor Author

Hey @arendjr ! I see that test failure, and I'm currently working to resolve it. Thank you for the speedy review!

Copy link
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

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

In order for the tests to work, biome_service needs to know how to handle the file. You'll need to add the language to the DocumentFileSource enum. IIRC, can_parse is the actual function that's used.

You should go ahead and add a experimental-grit feature flag too, just like how html is set up. You can use #3967 for reference.

@branberry
Copy link
Contributor Author

@dyc3 Perfect, thank you! I started going down that path and I was looking in biome_service, but I couldn't find exactly where I needed to make the change. Thanks for the reference!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter A-Parser Area: parser A-Project Area: project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants