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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: setup formbricks/js e2e tests #1794

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

ShubhamPalriwala
Copy link
Contributor

What does this PR do?

E2E tests for

  • Survey Creation
  • Display Creation through JS Package
  • Response Submission through JS Package
  • Verification of the above 2 from the Admin Portal

Also adds a step in CI to build the local JS package

Also uses the native HTML file to keep things light in the CI!

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change adds a new database migration
  • This change requires a documentation update

How should this be tested?

  • Test A
  • Test B

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read How we Code at Formbricks
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues
  • First PR at Formbricks? Please sign the CLA! Without it we wont be able to merge it 馃檹

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

Copy link

vercel bot commented Dec 18, 2023

The latest updates on your projects. Learn more about Vercel for Git 鈫楋笌

2 Ignored Deployments
Name Status Preview Updated (UTC)
formbricks-cloud 猬滐笍 Ignored (Inspect) Dec 18, 2023 6:19pm
formbricks-com 猬滐笍 Ignored (Inspect) Dec 18, 2023 6:19pm

Copy link
Contributor

Thank you for following the naming conventions for pull request titles! 馃檹

Copy link
Contributor

apps/web/playwright/utils/helper.ts

The current method of replacing the environment ID in the HTML file involves reading the entire file into memory, performing the replacement, and then writing the entire file back to disk. This can be inefficient for large files. A more efficient method would be to use a stream-based approach to read, modify, and write the file in chunks.
Create Issue
See the diff
Checkout the fix

    // In helper.ts
    import { createReadStream, createWriteStream } from 'fs';
    import { Transform } from 'stream';

    export const replaceEnvironmentIdInHtml = (filePath: string, environmentId: string): string => {
      const readStream = createReadStream(filePath, 'utf-8');
      const writeStream = createWriteStream(filePath);
      const transformStream = new Transform({
        transform(chunk, encoding, callback) {
          const replacedChunk = chunk.toString().replace(/environmentId: ".*?"/, `environmentId: "${environmentId}"`);
          callback(null, replacedChunk);
        }
      });

      readStream.pipe(transformStream).pipe(writeStream);
      return "file:///" + filePath;
    };
git fetch origin && git checkout -b ReviewBot/Impro-lenewie origin/ReviewBot/Impro-lenewie

Comment on lines +47 to +53
export const replaceEnvironmentIdInHtml = (filePath: string, environmentId: string): string => {
let htmlContent = readFileSync(filePath, "utf-8");
htmlContent = htmlContent.replace(/environmentId: ".*?"/, `environmentId: "${environmentId}"`);

writeFileSync(filePath, htmlContent);
return "file:///" + filePath;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

The current method of replacing the environment ID in the HTML file involves reading the entire file into memory, performing the replacement, and then writing the entire file back to disk. This can be inefficient for large files. A more efficient method would be to use a stream-based approach to read, modify, and write the file in chunks.

Suggested change
export const replaceEnvironmentIdInHtml = (filePath: string, environmentId: string): string => {
let htmlContent = readFileSync(filePath, "utf-8");
htmlContent = htmlContent.replace(/environmentId: ".*?"/, `environmentId: "${environmentId}"`);
writeFileSync(filePath, htmlContent);
return "file:///" + filePath;
};
import { createReadStream, createWriteStream } from 'fs';
import { Transform } from 'stream';
export const replaceEnvironmentIdInHtml = (filePath: string, environmentId: string): string => {
const readStream = createReadStream(filePath, 'utf-8');
const writeStream = createWriteStream(filePath);
const transformStream = new Transform({
transform(chunk, encoding, callback) {
const replacedChunk = chunk.toString().replace(/environmentId: ".*?"/, `environmentId: "${environmentId}"`);
callback(null, replacedChunk);
}
});
readStream.pipe(transformStream).pipe(writeStream);
return "file:///" + filePath;
};

Copy link
Member

@mattinannt mattinannt left a comment

Choose a reason for hiding this comment

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

@ShubhamPalriwala thank you for the new test; looks great! 馃槉馃挭

@mattinannt mattinannt added this pull request to the merge queue Dec 19, 2023
Merged via the queue into main with commit 45f02fd Dec 19, 2023
14 checks passed
@mattinannt mattinannt deleted the shubham/for-1562-add-e2e-test-for-formbricks-js branch December 19, 2023 08:13
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.

None yet

2 participants