Skip to content

FIX THE TESTS ON THIS REPO AKA SOLVE MY OCD#38

Merged
Pasithea0 merged 2 commits intop-stream:masterfrom
dumbutdumber:patch-1
Mar 2, 2026
Merged

FIX THE TESTS ON THIS REPO AKA SOLVE MY OCD#38
Pasithea0 merged 2 commits intop-stream:masterfrom
dumbutdumber:patch-1

Conversation

@dumbutdumber
Copy link
Contributor

Description

Just added npm install tweetnacl cause the TEST KEEPS FAILING and it annoys me
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

@qodo-code-review
Copy link

Review Summary by Qodo

Add tweetnacl npm install to integration test workflow

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Add npm install tweetnacl to account creation test
• Ensures tweetnacl dependency available before crypto operations
• Fixes intermittent test failures due to missing dependency
Diagram
flowchart LR
  A["Account Creation Test"] -->|Missing tweetnacl| B["Test Fails"]
  C["npm install tweetnacl"] -->|Install dependency| A
  A -->|Dependency available| D["Test Passes"]
Loading

Grey Divider

File Changes

1. .github/workflows/pull-request-testing.yaml 🐞 Bug fix +2/-0

Add tweetnacl dependency installation to test workflow

• Added npm install tweetnacl command before account creation integration test
• Ensures tweetnacl crypto library is installed before test execution
• Includes explanatory comment about dependency installation requirement

.github/workflows/pull-request-testing.yaml


Grey Divider

Qodo Logo

@qodo-code-review
Copy link

qodo-code-review bot commented Feb 27, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. tweetnacl still not resolved🐞 Bug ✓ Correctness
Description
The integration test script is created at /tmp/auth-test.mjs and imports tweetnacl, but the PR
installs tweetnacl into the repo workspace, so the script may still fail to resolve the module.
This makes the PR’s fix ineffective and can keep CI failing with a module-resolution error.
Code

.github/workflows/pull-request-testing.yaml[R98-101]

+          npm install tweetnacl
         # Create a Node.js script to handle the crypto operations
         cat > /tmp/auth-test.mjs << 'EOF'
         import crypto from 'crypto';
Evidence
The workflow installs dependencies in the repository workspace, but the auth test script is written
to /tmp and executed from there while importing tweetnacl as a bare module specifier. Installing
into the workspace doesn’t guarantee that a script located in /tmp can resolve that dependency.

.github/workflows/pull-request-testing.yaml[39-41]
.github/workflows/pull-request-testing.yaml[95-103]
.github/workflows/pull-request-testing.yaml[271-274]
package.json[20-31]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow writes the Node integration test to `/tmp/auth-test.mjs` but installs dependencies in the repository workspace. This can prevent `import nacl from &amp;#x27;tweetnacl&amp;#x27;` from resolving, so adding `npm install tweetnacl` may not actually fix the failing test.
### Issue Context
- `tweetnacl` is already a declared dependency in `package.json` and present in `package-lock.json`.
- The script is executed from `/tmp`, not from the workspace directory.
### Fix Focus Areas
- .github/workflows/pull-request-testing.yaml[95-103]
### Suggested change
Option A (recommended): write/run the script in the workspace:
- Replace `cat &amp;gt; /tmp/auth-test.mjs` with `cat &amp;gt; auth-test.mjs` (or `${{ github.workspace }}/auth-test.mjs`)
- Replace `node /tmp/auth-test.mjs` with `node auth-test.mjs`
- Remove the extra `npm install tweetnacl` line
Option B: if you must use `/tmp`, install deps into `/tmp`:
- `npm install --prefix /tmp tweetnacl` (and ensure `/tmp/node_modules` exists for resolution)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Redundant ad-hoc npm install🐞 Bug ⛯ Reliability
Description
tweetnacl is already in package.json/package-lock.json, and the workflow already runs `npm
install`, so installing it again is redundant. This adds extra network/time cost and can reduce CI
determinism if it mutates the lockfile or pulls a different resolved version than expected.
Code

.github/workflows/pull-request-testing.yaml[R97-98]

+          # Making sure this is installed cause it keeeps failing cause of this
+          npm install tweetnacl
Evidence
The workflow already installs all dependencies and the repository already declares/locks
tweetnacl, so adding a second install is unnecessary and can introduce drift versus the
lockfile-resolved tree.

.github/workflows/pull-request-testing.yaml[39-41]
.github/workflows/pull-request-testing.yaml[97-99]
package.json[20-31]
package-lock.json[6796-6799]
server/utils/challenge.ts[1-4]
server/routes/auth/derive-public-key.post.ts[1-4]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow runs `npm install tweetnacl` even though the repo already installs dependencies and already declares/locks `tweetnacl`. This is redundant and can add variability/time.
### Issue Context
- `tweetnacl` is already in `package.json` and resolved in `package-lock.json`.
- The workflow already runs `npm install` earlier.
### Fix Focus Areas
- .github/workflows/pull-request-testing.yaml[97-99]
- .github/workflows/pull-request-testing.yaml[39-41]
### Suggested change
- Delete `npm install tweetnacl` from the integration test step.
- (Optional CI hardening) Replace `npm install` with `npm ci` in the dependency install step to ensure lockfile-deterministic installs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@dumbutdumber
Copy link
Contributor Author

Fixed this by removing it from temp and placing it in root but also rm it after its use. I still think this is an ugly implementation. Another method is node --input-type=module according to some github issues i have seen but idk if this approach is that much better. To whoever reviews this: if u want me to implement the -input-type=module I can do that instead.

@dumbutdumber
Copy link
Contributor Author

HOW TF DID THIS TEST WORK???? ...OH WAIT it uses my code lol. Ig we can confirm it works :))

@dumbutdumber dumbutdumber changed the title Update pull-request-testing.yaml FIX THE TESTS ON THIS REPO AKA SOLVE MY OCD Feb 27, 2026
@Duplicake-fyi Duplicake-fyi self-requested a review March 1, 2026 15:07
Copy link

@Duplicake-fyi Duplicake-fyi left a comment

Choose a reason for hiding this comment

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

yeah this looks good, seems like you confimed it works and it's a very simple change

@Duplicake-fyi
Copy link

@Pasithea0 this can be merged

@Pasithea0 Pasithea0 merged commit c024414 into p-stream:master Mar 2, 2026
1 check passed
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.

3 participants