headway: fix async-seed test flakes killing CI - #1496
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe test harnesses now use async Tokio tests and subscription notifications. Private relay state now tracks authoritative kind-10013 creation timestamps during loading, polling, and local updates. ChangesDeterministic async test synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AsyncTest
participant WaitUntil
participant SubscriptionStream
participant Headway
AsyncTest->>WaitUntil: await seeded-state predicate
WaitUntil->>SubscriptionStream: await ingestion notification
SubscriptionStream-->>WaitUntil: return ingestion batch
WaitUntil->>Headway: fold batch
Headway-->>WaitUntil: report board or card state
WaitUntil-->>AsyncTest: return when predicate matches
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jb55
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/headway/src/store.rs (1)
1087-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the modified test.
Add a Rust doc comment above
seed_demo_materialises_cards. State that the test waits for the amended card title before checking materialized card counts.Proposed change
#[test] +/// Waits for the amended seed card before checking materialized card counts. fn seed_demo_materialises_cards() {As per coding guidelines: “Ensure docstring coverage for any code added or modified.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/headway/src/store.rs` around lines 1087 - 1099, Add a Rust doc comment (using ///) above the seed_demo_materialises_cards test function that explains the test waits for the amended card title "Define nostr event model for boards" before asserting the card count. Reference that this approach avoids a race condition where checking only the card count would occur before the title amendment is ingested.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/headway/src/store.rs`:
- Around line 1087-1099: Add a Rust doc comment (using ///) above the
seed_demo_materialises_cards test function that explains the test waits for the
amended card title "Define nostr event model for boards" before asserting the
card count. Reference that this approach avoids a race condition where checking
only the card count would occur before the title amendment is ingested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a423dd43-98c9-423e-b666-f3e8b90efe4c
📒 Files selected for processing (1)
crates/headway/src/store.rs
jb55
changed the title
jb55
force-pushed
the
fix/flaky-board-cache-finalizes
branch
from
August 4, 2026 18:16
f35c819 to
5c5f17b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/headway/src/store.rs`:
- Around line 1092-1093: Add behavior-focused Rust doc comments immediately
before each modified #[tokio::test] declaration in crates/headway/src/store.rs:
seed_materialises_default_board (1092-1093), the demo-card seed test
(1106-1107), append-card test (1132-1133), label-application test (1152-1153),
publisher-frame test (1184-1185), cross-column move test (1225-1226),
title/description/label editing test (1248-1249), comment/reply folding test
(1295-1296), deletion test (1357-1358), archive/restore test (1370-1371),
column-operation round-trip test (1401-1402), board-rename preservation test
(1436-1437), linked-card placement test (1538-1539), cross-board relocation test
(1572-1573), target-column preservation test (1602-1603), and first-column
fallback test (1647-1648); each summary should describe the stated behavior and
use Rust /// syntax.
- Around line 1058-1063: The await_ingest() helpers in
crates/headway/src/store.rs (1058-1063) and crates/notedeck_headway/src/lib.rs
(1106-1111) can wait indefinitely on SubscriptionStream::next(). Add an idle
timeout around each next() that resets after progress, plus an overall deadline
to bound waits when unrelated batches continue arriving; preserve the existing
failure behavior when the stream closes or either timeout expires.
In `@crates/notedeck_headway/src/lib.rs`:
- Around line 1113-1115: Add a Rust `///` documentation comment immediately
above `total_cards` stating that it counts the cards across all board columns,
without changing the function’s implementation.
- Around line 1392-1395: Update the ingest loop around fold and await_ingest to
use demo_seed_complete as the termination predicate instead of checking whether
total_cards equals seven. Continue awaiting stream ingestion until the complete
terminal seed state is reached.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a10e41bb-9488-4c4d-b3f9-69381f35d1df
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/headway/Cargo.tomlcrates/headway/src/store.rscrates/notedeck_headway/Cargo.tomlcrates/notedeck_headway/src/lib.rs
Comment on lines
+1092
to
+1093
| #[tokio::test] | ||
| async fn seed_materialises_default_board() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add Rust doc comments to the modified store tests.
These changed test declarations have no /// summary. Add one behavior-focused summary before each #[tokio::test].
crates/headway/src/store.rs#L1092-L1093: document default-board materialization.crates/headway/src/store.rs#L1106-L1107: document demo-card seed materialization.crates/headway/src/store.rs#L1132-L1133: document appending a card.crates/headway/src/store.rs#L1152-L1153: document label application.crates/headway/src/store.rs#L1184-L1185: document publisher frame emission.crates/headway/src/store.rs#L1225-L1226: document the cross-column card move.crates/headway/src/store.rs#L1248-L1249: document title, description, and label editing.crates/headway/src/store.rs#L1295-L1296: document comment and reply folding.crates/headway/src/store.rs#L1357-L1358: document card deletion.crates/headway/src/store.rs#L1370-L1371: document archive and restore behavior.crates/headway/src/store.rs#L1401-L1402: document the column-operation round trip.crates/headway/src/store.rs#L1436-L1437: document board rename preservation.crates/headway/src/store.rs#L1538-L1539: document linked-card placement.crates/headway/src/store.rs#L1572-L1573: document cross-board relocation.crates/headway/src/store.rs#L1602-L1603: document target-column preservation.crates/headway/src/store.rs#L1647-L1648: document first-column fallback.
As per coding guidelines, ensure docstring coverage for any code added or modified.
📍 Affects 1 file
crates/headway/src/store.rs#L1092-L1093(this comment)crates/headway/src/store.rs#L1106-L1107crates/headway/src/store.rs#L1132-L1133crates/headway/src/store.rs#L1152-L1153crates/headway/src/store.rs#L1184-L1185crates/headway/src/store.rs#L1225-L1226crates/headway/src/store.rs#L1248-L1249crates/headway/src/store.rs#L1295-L1296crates/headway/src/store.rs#L1357-L1358crates/headway/src/store.rs#L1370-L1371crates/headway/src/store.rs#L1401-L1402crates/headway/src/store.rs#L1436-L1437crates/headway/src/store.rs#L1538-L1539crates/headway/src/store.rs#L1572-L1573crates/headway/src/store.rs#L1602-L1603crates/headway/src/store.rs#L1647-L1648
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/headway/src/store.rs` around lines 1092 - 1093, Add behavior-focused
Rust doc comments immediately before each modified #[tokio::test] declaration in
crates/headway/src/store.rs: seed_materialises_default_board (1092-1093), the
demo-card seed test (1106-1107), append-card test (1132-1133), label-application
test (1152-1153), publisher-frame test (1184-1185), cross-column move test
(1225-1226), title/description/label editing test (1248-1249), comment/reply
folding test (1295-1296), deletion test (1357-1358), archive/restore test
(1370-1371), column-operation round-trip test (1401-1402), board-rename
preservation test (1436-1437), linked-card placement test (1538-1539),
cross-board relocation test (1572-1573), target-column preservation test
(1602-1603), and first-column fallback test (1647-1648); each summary should
describe the stated behavior and use Rust /// syntax.
Source: Coding guidelines
Comment on lines
1113
to
1115
| fn total_cards(view: &BoardView) -> usize { | ||
| view.columns.iter().map(|c| c.cards.len()).sum() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document total_cards.
total_cards is new and has no Rust doc comment. Add a /// summary that states it counts cards across all board columns.
As per coding guidelines, ensure docstring coverage for any code added or modified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/notedeck_headway/src/lib.rs` around lines 1113 - 1115, Add a Rust
`///` documentation comment immediately above `total_cards` stating that it
counts the cards across all board columns, without changing the function’s
implementation.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/notedeck/src/account/relay.rs (2)
664-670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the supplied creation timestamp.
This test passes
created_atbut does not verify that the note stores it. Assertnote.created_at()so a regression that ignores the new parameter fails.Proposed test assertion
assert_eq!(note.kind(), PRIVATE_RELAY_LIST_KIND); + assert_eq!(note.created_at(), 1_700_000_000); // The relay set must not leak into the public content or tags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/notedeck/src/account/relay.rs` around lines 664 - 670, Update the test around construct_private_relay_list_note to assert that note.created_at() equals the supplied timestamp 1_700_000_000, while preserving the existing kind, content, and tag assertions.
20-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign and complete the timestamp documentation.
Lines 20-25 say polling rejects timestamps at or below
private_updated_at. Line 216 rejects only strictly older timestamps. State the actual equal-timestamp rule. Add Rustdoc for both changed constructors. Specify thatcreated_atuses Unix seconds and document the ordering requirement.As per coding guidelines, ensure docstring coverage for any code added or modified.
Also applies to: 133-138, 343-357
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/notedeck/src/account/relay.rs` around lines 20 - 25, Align the relay timestamp documentation with the implementation: document that equal created_at values are accepted while only strictly older values are rejected, and state that created_at is Unix seconds with each authoritative update strictly greater than private_updated_at. Add Rustdoc to both modified constructors, covering their behavior and the same timestamp-ordering requirement.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/notedeck/src/account/relay.rs`:
- Around line 562-572: In the private relay update flow surrounding
new_private_relay_list_note, only advance account_data.relay.private_updated_at
after confirming kp.secret_key is present. Ensure read-only accounts return
without mutating the private-update floor, while retaining the existing
timestamp and note-generation behavior for accounts with a secret key.
---
Nitpick comments:
In `@crates/notedeck/src/account/relay.rs`:
- Around line 664-670: Update the test around construct_private_relay_list_note
to assert that note.created_at() equals the supplied timestamp 1_700_000_000,
while preserving the existing kind, content, and tag assertions.
- Around line 20-25: Align the relay timestamp documentation with the
implementation: document that equal created_at values are accepted while only
strictly older values are rejected, and state that created_at is Unix seconds
with each authoritative update strictly greater than private_updated_at. Add
Rustdoc to both modified constructors, covering their behavior and the same
timestamp-ordering requirement.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b64152dd-cc10-43ea-8863-8bd00f5074e3
📒 Files selected for processing (1)
crates/notedeck/src/account/relay.rs
Comment on lines
+562
to
+572
| // Keep republishes strictly newer than the last one this device wrote so an | ||
| // add immediately followed by a remove (same wall-clock second) still | ||
| // resolves to the remove, rather than relying on NIP-01's id tie-break. | ||
| let created_at = crate::unix_time_secs().max(account_data.relay.private_updated_at + 1); | ||
| account_data.relay.private_updated_at = created_at; | ||
|
|
||
| // Encrypt + sign the kind-10013 list. None for a read-only account. | ||
| let Some(note) = account_data.relay.new_private_relay_list_note(kp) else { | ||
| let Some(note) = account_data | ||
| .relay | ||
| .new_private_relay_list_note(kp, created_at) | ||
| else { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline crates/notedeck/src/account/relay.rs --items all --match modify_private_relays
rg -n -C 8 '\bmodify_private_relays\s*\(' crates/notedeck/src
rg -n -C 4 'Keypair::only_pubkey|read.?only' crates/notedeck/srcRepository: damus-io/notedeck
Length of output: 16251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant relay.rs sections =="
sed -n '485,590p' crates/notedeck/src/account/relay.rs
echo
echo "== polling and private_updated_at usages =="
rg -n -C 5 'private_updated_at|parse_private_relay_list_note|new_private_relay_list_note|selected_account_private_relays' crates/notedeck/src
echo
echo "== RelayAction and private action handling =="
rg -n -C 6 'enum RelayAction|is_private|AddPrivate|RemovePrivate|Action' crates/notedeck/src/account/relay.rs crates/notedeck/src/account -g '*.rs'Repository: damus-io/notedeck
Length of output: 48353
Do not advance the private-update floor without a secret key.
A read-only account cannot construct the kind-10013 list, but this path still raises private_updated_at before new_private_relay_list_note returns None. That keeps valid private lists produced by another device at or below the local floor and prevents polling from adopting them. Guard kp.secret_key.is_some() before mutating private relay state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/notedeck/src/account/relay.rs` around lines 562 - 572, In the private
relay update flow surrounding new_private_relay_list_note, only advance
account_data.relay.private_updated_at after confirming kp.secret_key is present.
Ensure read-only accounts return without mutating the private-update floor,
while retaining the existing timestamp and note-generation behavior for accounts
with a secret key.
jb55
force-pushed
the
fix/flaky-board-cache-finalizes
branch
from
August 4, 2026 22:40
9083c04 to
7ee306f
Compare
Same class of seed-race flake as board_cache_finalizes_once_per_fold: the
test asserts the first backlog card's title is the *amended* value
("Define nostr event model for boards"), but only waited for total_cards == 7.
seed_demo_board seeds that card as "Nostr event model" and renames it with a
subject edit ingested after all seven cards land, so the count predicate can
be satisfied a frame before the rename folds in, leaving the card at its
original title and failing the assert.
Wait for the renamed title itself. The amendment folds after every card, so
its presence also implies all seven cards are here; the count assertions stay
as-is.
Fixes: 1209d11 ("headway: derive a Linear-style activity timeline from board events")
Changelog-None:
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The async-seed test harnesses (TestSync in notedeck_headway, TestNdb in headway::store) polled the writer against a fixed wall-clock deadline, so a starved-but-live writer on a loaded CI runner could blow past it — the intermittent "predicate never held" failures (e.g. poll_folds_changes_as_a_delta). A wall-clock budget can't reliably wait on an async ingest. Drive the fold loops off nostrdb's own subscription notification instead: `wait`/`poll_board` open a SubscriptionStream and, between folds, `await` the writer delivering the next note. No deadline, no sleep — the loop wakes exactly when there's more to fold and blocks otherwise. The tests become `#[tokio::test]`. Also replaces the old `drain()` quiescence guess (which returned on the first empty poll, mid-materialisation) with `demo_seed_complete`: the demo seed's last event moves the drag card into In Progress, so that card in column 2 is a deterministic "fully folded" signal. Verified under full CPU saturation (which failed the wall-clock versions 40/40): poll_folds_changes_as_a_delta and seed_demo_materialises_cards now pass 15/15. Fixes: b79bff4 ("notedeck_headway: memoize the folded board per frame") Changelog-None: Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…le re-delivery Clearing (or retargeting) a private relay marker republishes an empty kind-10013 NIP-37 list, but `poll_private_for_updates` harvested whatever kind-10013 note ndb had just ingested. An out-of-order re-delivery of the older AddPrivate note therefore resurrected the private set, flipping dave's PNS sync back on and emitting a second NEG-OPEN — the deterministic `dave_pns_clears_configured_relay_when_private_marker_removed_e2e` / `dave_pns_retargets_when_configured_relay_changes_e2e` failures. Three composing parts: - Poll re-resolves the canonical latest replaceable note instead of trusting the arrived note keys. - Republishes stamp a strictly monotonic `created_at` (`unix_time_secs().max( private_updated_at + 1)`) so a same-second remove supersedes the add rather than relying on NIP-01's non-deterministic lowest-id tie-break. - `private_updated_at` becomes an authoritative read-side floor: a poll that resolves a note at a `created_at` below our last local write is a stale re-delivery and is ignored, closing the window where local ndb still holds only the older note until the empty one round-trips back. Fixes: c7c14be ("relay: store private-sync relays as a kind-10013 NIP-37 list") Changelog-Fixed: Fix a cleared private (NIP-37) relay being resurrected by a stale replaceable-list re-delivery Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ions
Each live/remote session opened its own nostrdb subscription for kind-1988
conversation events (filtered by kind + author + the session's `d`-tag).
nostrdb caps subscriptions at 256 per Ndb, so once an account accumulated
that many sessions the subscribe calls started failing ("too many
subscriptions"), silently dropping live conversation and permission events
for every session past the cap.
Replace the per-session subscriptions with two shared per-account cursors
over all kind-1988 events (`conversation_sub` for chat sync,
`conversation_action_sub` for permission responses / mode commands — two
cursors because they poll at different points in the frame). Each consumer
polls its cursor once and demuxes the returned notes to the owning session
by `d`-tag via `conversation_session_index`. One pair of subscriptions now
serves any number of sessions.
Removes the per-session `live_conversation_sub`/`conversation_action_sub`
fields, the `setup_conversation_subscription`/`setup_conversation_action_subscription`
helpers, and the now-unused `ConversationSubscriptionScope` plumbing through
`create_session_with_cwd`.
The 500-session `pns_outbox_e2e` stress binary now runs with 0 "too many
subscriptions" warnings (was 252) and all 19 tests pass.
Changelog-Fixed: Dave no longer drops live conversation events once an account has many sessions (nostrdb subscription cap)
jb55
force-pushed
the
fix/flaky-board-cache-finalizes
branch
from
August 4, 2026 22:41
7ee306f to
a239586
Compare
…oard `total_cards(v) == 7` can hold mid-fold, before the demo seed's last event (the drag card landing in In Progress) settles, so the follow-up layout assertion occasionally ran against a half-materialised board — the flake seen on macOS/Windows CI (`left: 5, right: 3`). Wait on `demo_seed_complete` instead: the drag card sitting in column 2 is the seed's terminal state, so every earlier event has already folded. This matches the deterministic signal the other sync tests already use. Changelog-None:
The `test_real_codex_*` integration tests are `#[ignore]`d, but the snapshot
CI job runs every ignored test in the notedeck_dave lib via `--ignored` (see
scripts/snapshot-test), which sweeps these real-binary tests up alongside the
lavapipe snapshots. On runners without `codex` on PATH they panicked at spawn
("Failed to spawn codex app-server — is codex installed?"), reddening the job.
Have `setup_real_codex_test` return `None` when `spawn_codex` fails so the
tests skip rather than fail. The real path still runs locally when codex is
installed; a missing binary is now a visible skip, not a hard failure.
Changelog-None:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Several headway tests read seeded board state before the async nostrdb writer finished folding the whole seed, so they passed or failed on writer timing — flaky in CI. Three commits.
1.
notedeck_headway::board_cache_finalizes_once_per_foldFailing e.g. in run 30780707923. It stopped folding at
total_cards == 7, butseed_demokeeps ingesting after the 7th card (parent relations, card amendments, the drag-card move into In Progress). One arriving during the "steady frame" read loop re-finalized the memo → assertion failed. Fix: wait for the drag move (the seed's last event) to land in In Progress before measuring.2.
headway::store::seed_demo_materialises_cardsAsserts the first card's amended title but only waited for
total_cards == 7; the rename is ingested after all seven cards, so the count could hold a frame before the rename folded. Fix: wait for the renamed title itself.3. The harnesses themselves (
TestSync,TestNdb) — the real root causeBoth polled the writer against a fixed 5s wall-clock deadline, which a starved-but-live writer on a loaded CI runner blows past (the intermittent
poll_folds_changes_as_a_delta"predicate never held"). A wall-clock budget fundamentally can't wait on an async ingest.Drive the fold loops off nostrdb's own subscription notification instead:
wait/poll_boardopen aSubscriptionStreamandawaitthe writer delivering the next note between folds — no deadline, no sleep, wakes exactly when there's more to fold. Tests become#[tokio::test]. Also replaces the olddrain()quiescence guess withdemo_seed_complete(drag card in In Progress = the seed's last event = fully folded).Verification
notedeck_headway61/61,headway31/31;clippy --all-targets+fmtclean.poll_folds_changes_as_a_deltaandseed_demo_materialises_cards.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests