Skip to content

Commit

Permalink
chore: stabilise tests on macOS
Browse files Browse the repository at this point in the history
  • Loading branch information
@arendjr
arendjr committed Mar 4, 2025
1 parent 6a36760 commit 58df41d
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 82 deletions.
2 changes: 1 addition & 1 deletion crates/biome_cli/src/commands/daemon.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ pub(crate) fn run_server(
) -> Result<(), CliDiagnostic> {
setup_tracing_subscriber(log_path.as_deref(), log_file_name_prefix.as_deref());

let (mut watcher, instruction_channel) = WorkspaceWatcher::new()?;
let (mut watcher, instruction_channel, _) = WorkspaceWatcher::new()?;

let rt = Runtime::new()?;
let factory = ServerFactory::new(stop_on_disconnect, instruction_channel.sender.clone());
Expand Down
135 changes: 73 additions & 62 deletions crates/biome_lsp/src/server.tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2953,6 +2953,25 @@ async fn pull_source_assist_action() -> Result<()> {

#[tokio::test]
async fn watcher_updates_dependency_graph() -> Result<()> {
const FOO_CONTENT: &str = r#"import { bar } from "./bar.ts";
export function foo() {
bar();
}
"#;
const BAR_CONTENT: &str = r#"import { foo } from "./foo.ts";
export function bar() {
foo();
}
"#;
const BAR_CONTENT_FIXED: &str = r#"import { foo } from "./shared.ts";
export function bar() {
foo();
}
"#;

// ARRANGE: Set up FS and LSP connection in order to test import cycles.
let mut fs = TemporaryFs::new("watcher_updates_dependency_graph");
fs.create_file(
Expand All @@ -2970,27 +2989,10 @@ async fn watcher_updates_dependency_graph() -> Result<()> {
"#,
);

fs.create_file(
"foo.ts",
r#"import { bar } from "./bar.ts";
fs.create_file("foo.ts", FOO_CONTENT);
fs.create_file("bar.ts", BAR_CONTENT);

export function foo() {
bar();
}
"#,
);

fs.create_file(
"bar.ts",
r#"import { foo } from "./foo.ts";
export function bar() {
foo();
}
"#,
);

let (mut watcher, instruction_channel) = WorkspaceWatcher::new()?;
let (mut watcher, instruction_channel, notification_channel) = WorkspaceWatcher::new()?;

let factory = ServerFactory::new(true, instruction_channel.sender.clone());

Expand Down Expand Up @@ -3060,12 +3062,15 @@ export function bar() {
"This import is part of a cycle."
);

let _ = notification_channel.receiver.try_recv(); // Clear notification, if any.

// ARRANGE: Remove `bar.ts`.
std::fs::remove_file(fs.working_directory.join("bar.ts")).expect("Cannot remove bar.ts");

// FIXME: If this test is unstable, we may need to wait here.
// Right now, we don't know if the watcher already processed the
// removal. It seems everything works fine though :D
notification_channel
.receiver
.recv()
.expect("Expected notification");

// ACT: Pull diagnostics.
let result: PullDiagnosticsResult = server
Expand All @@ -3089,15 +3094,14 @@ export function bar() {
assert_eq!(result.diagnostics.len(), 0);

// ARRANGE: Recreate `bar.ts`.
fs.create_file(
"bar.ts",
r#"import { foo } from "./foo.ts";
let _ = notification_channel.receiver.try_recv(); // Clear notification, if any.

export function bar() {
foo();
}
"#,
);
fs.create_file("bar.ts", BAR_CONTENT);

notification_channel
.receiver
.recv()
.expect("Expected notification");

// ACT: Pull diagnostics.
let result: PullDiagnosticsResult = server
Expand Down Expand Up @@ -3125,15 +3129,14 @@ export function bar() {
);

// ARRANGE: Fix `bar.ts`.
fs.create_file(
"bar.ts",
r#"import { foo } from "./shared.ts";
let _ = notification_channel.receiver.try_recv(); // Clear notification, if any.

export function bar() {
foo();
}
"#,
);
fs.create_file("bar.ts", BAR_CONTENT_FIXED);

notification_channel
.receiver
.recv()
.expect("Expected notification");

// ACT: Pull diagnostics.
let result: PullDiagnosticsResult = server
Expand All @@ -3156,6 +3159,7 @@ export function bar() {
// ASSERT: Diagnostic should disappear again with a fixed `bar.ts`.
assert_eq!(result.diagnostics.len(), 0);

let _ = instruction_channel.sender.send(WatcherInstruction::Stop);
server.shutdown().await?;
reader.abort();

Expand All @@ -3164,6 +3168,19 @@ export function bar() {

#[tokio::test]
async fn watcher_updates_dependency_graph_with_directories() -> Result<()> {
const FOO_CONTENT: &str = r#"import { bar } from "./utils/bar.ts";
export function foo() {
bar();
}
"#;
const BAR_CONTENT: &str = r#"import { foo } from "../foo.ts";
export function bar() {
foo();
}
"#;

// ARRANGE: Set up FS and LSP connection in order to test import cycles.
let mut fs = TemporaryFs::new("watcher_updates_dependency_graph_with_directories");
fs.create_file(
Expand All @@ -3181,27 +3198,10 @@ async fn watcher_updates_dependency_graph_with_directories() -> Result<()> {
"#,
);

fs.create_file(
"foo.ts",
r#"import { bar } from "./utils/bar.ts";
export function foo() {
bar();
}
"#,
);

fs.create_file(
"utils/bar.ts",
r#"import { foo } from "../foo.ts";
fs.create_file("foo.ts", FOO_CONTENT);
fs.create_file("utils/bar.ts", BAR_CONTENT);

export function bar() {
foo();
}
"#,
);

let (mut watcher, instruction_channel) = WorkspaceWatcher::new()?;
let (mut watcher, instruction_channel, notification_channel) = WorkspaceWatcher::new()?;

let factory = ServerFactory::new(true, instruction_channel.sender.clone());

Expand Down Expand Up @@ -3271,16 +3271,19 @@ export function bar() {
"This import is part of a cycle."
);

let _ = notification_channel.receiver.try_recv(); // Clear notification, if any.

// ARRANGE: Move `utils` directory.
std::fs::rename(
fs.working_directory.join("utils"),
fs.working_directory.join("bin"),
)
.expect("Cannot move utils");

// FIXME: If this test is unstable, we may need to wait here.
// Right now, we don't know if the watcher already processed the
// move. It seems everything works fine though :D
notification_channel
.receiver
.recv()
.expect("Expected notification");

// ACT: Pull diagnostics.
let result: PullDiagnosticsResult = server
Expand All @@ -3305,12 +3308,19 @@ export function bar() {
assert_eq!(result.diagnostics.len(), 0);

// ARRANGE: Move `utils` back.
let _ = notification_channel.receiver.try_recv(); // Clear notification, if any.

std::fs::rename(
fs.working_directory.join("bin"),
fs.working_directory.join("utils"),
)
.expect("Cannot restore utils");

notification_channel
.receiver
.recv()
.expect("Expected notification");

// ACT: Pull diagnostics.
let result: PullDiagnosticsResult = server
.request(
Expand All @@ -3336,6 +3346,7 @@ export function bar() {
"This import is part of a cycle."
);

let _ = instruction_channel.sender.send(WatcherInstruction::Stop);
server.shutdown().await?;
reader.abort();

Expand Down
63 changes: 44 additions & 19 deletions crates/biome_service/src/workspace_watcher.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ pub enum WatcherInstruction {
Stop,
}

/// Channel for receiving notifications when the watcher has processed events.
pub struct WatcherNotificationChannel {
pub receiver: Receiver<()>,
}

/// Channel for sending instructions to the watcher.
///
/// Only exposes the sender of the channel.
Expand Down Expand Up @@ -73,14 +78,20 @@ pub struct WorkspaceWatcher {

/// Channel receiver for watch instructions.
instruction_rx: Receiver<WatcherInstruction>,

/// Channel sender for sending notifications on the
/// [WatcherNotificationChannel].
notification_tx: Sender<()>,
}

impl WorkspaceWatcher {
/// Constructor.
///
/// Returns both the watcher as well as the channel for sending instructions
/// to the watcher.
pub fn new() -> Result<(Self, WatcherInstructionChannel), WorkspaceError> {
/// Returns the watcher as well as two channel endpoints: one for sending
/// instructions to the watcher, and one for receiving notifications when
/// the watcher has processed events.
pub fn new()
-> Result<(Self, WatcherInstructionChannel, WatcherNotificationChannel), WorkspaceError> {
// We use a bounded channel, because watchers are
// [intrinsically unreliable](https://docs.rs/notify/latest/notify/#watching-large-directories).
// If we block the sender, some events may get dropped, but that was
Expand All @@ -94,16 +105,21 @@ impl WorkspaceWatcher {
let watcher = recommended_watcher(tx)?;

let (instruction_tx, instruction_rx) = unbounded();
let (notification_tx, notification_rx) = bounded(1);
let instruction_channel = WatcherInstructionChannel {
sender: instruction_tx,
};
let notification_channel = WatcherNotificationChannel {
receiver: notification_rx,
};
let watcher = Self {
watcher: Box::new(watcher),
notify_rx: rx,
instruction_rx,
notification_tx,
};

Ok((watcher, instruction_channel))
Ok((watcher, instruction_channel, notification_channel))
}

/// Runs the watcher.
Expand All @@ -113,7 +129,7 @@ impl WorkspaceWatcher {
/// end of the instructions channel) or the watcher (unexpectedly) stops.
/// Under normal operation, neither should happen before the daemon
/// terminates.
#[tracing::instrument(level = "debug", skip(self, workspace))]
//#[tracing::instrument(level = "debug", skip(self, workspace))]
pub fn run(&mut self, workspace: &WorkspaceServer) {
loop {
crossbeam::channel::select! {
Expand All @@ -123,39 +139,48 @@ impl WorkspaceWatcher {
debug!(event = debug(&event), "watcher_event");
}

let result = match event.kind {
EventKind::Access(_) => Ok(()),
EventKind::Create(create_kind) => match create_kind {
CreateKind::Folder => workspace.open_folders_through_watcher(event.paths),
let (is_processed, result) = match event.kind {
EventKind::Access(_) => (false, Ok(())),
EventKind::Create(create_kind) => (true, match create_kind {
CreateKind::Folder => {
workspace.open_folders_through_watcher(event.paths)
}
_ => workspace.open_paths_through_watcher(event.paths),
},
}),
EventKind::Modify(modify_kind) => match modify_kind {
// `ModifyKind::Any` needs to be included as a catch-all.
// Without it, we'll miss events on Windows.
ModifyKind::Data(_) | ModifyKind::Any => {
workspace.open_paths_through_watcher(event.paths)
(true, workspace.open_paths_through_watcher(event.paths))
},
ModifyKind::Name(RenameMode::From) => {
workspace.close_paths_through_watcher(event.paths)
},
(true, workspace.close_paths_through_watcher(event.paths))
}
ModifyKind::Name(RenameMode::To) => {
workspace.open_paths_through_watcher(event.paths)
(true, workspace.open_paths_through_watcher(event.paths))
},
ModifyKind::Name(RenameMode::Both) => {
workspace.rename_path_through_watcher(&event.paths[0], &event.paths[1])
(true, workspace.rename_path_through_watcher(
&event.paths[0],
&event.paths[1]
))
},
_ => Ok(()),
_ => (false, Ok(())),
},
EventKind::Remove(remove_kind) => match remove_kind {
EventKind::Remove(remove_kind) => (true, match remove_kind {
RemoveKind::File => workspace.close_files_through_watcher(event.paths),
_ => workspace.close_paths_through_watcher(event.paths),
},
EventKind::Any | EventKind::Other => Ok(()),
}),
EventKind::Any | EventKind::Other => (false, Ok(())),
};
if let Err(error) = result {
// TODO: Improve error propagation.
warn!("Error processing watch event: {error}");
}

if is_processed {
let _ = self.notification_tx.try_send(());
}
},
Ok(Err(error)) => {
// TODO: Improve error propagation.
Expand Down

0 comments on commit 58df41d

Please sign in to comment.