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

First pass at getting new UI testing down #4487

Merged
merged 21 commits into from
Aug 12, 2024
Merged

Conversation

tsmartt
Copy link
Contributor

@tsmartt tsmartt commented Jul 25, 2024

Resolves https://github.com/brave-intl/creators-private-issues/issues/1859

This PR sets up a separate set of Capybara tests to hit the new UI written in NextJS.
We add a test runner script that will run the old tests as well as the new UI tests.
They must be run separately as we redirect Capybara to an external host which breaks a bunch of the old tests.
Using this separate runner we can piecemeal port the old capybara tests over to the new UI.

@tsmartt tsmartt requested a review from jlbyrne August 7, 2024 01:09
@tsmartt
Copy link
Contributor Author

tsmartt commented Aug 7, 2024

This is ready to go except for an elliptic check which just popped up

See indutny/elliptic#317 for more

Will make a separate PR for that before merging this

@tsmartt tsmartt marked this pull request as ready for review August 7, 2024 01:35
@kdenhartog
Copy link
Member

kdenhartog commented Aug 8, 2024

From what I can tell, this is occurring because we need crypto-browserify>browserify-sign>elliptic as a webpack replacement for node crypto. However, it's not clear where this is actually in use when I did a quick search through the various JS code. I did also have a quick DM with @tsmartt to see if there was something I missed and it doesn't seem like it.

AFAICT we're only producing signatures on the JS side and that's done by calling into the wallet scripts injected by the wallet (which means the wallet code of the user is doing the signing not our frontend code), but not verifying them with JS. I did check through where we're actually doing verification of signatures and it appears to be using ruby instead so I don't believe we're hitting the vulnerable code paths. If for some reason you find that we are using crypto-browserify in someway and that is performing signature verification then it would present a small issue but not one that I think is feasibly exploitable.

So, I'm alright if we just ignore this update until the patch to the elliptic library is accepted and propagated through to crypto-browserify. Alternatively, since it doesn't seem like crypto-browserify is actually in use we could also look at dropping the dependency all together.

@tsmartt
Copy link
Contributor Author

tsmartt commented Aug 9, 2024

I looked at dropping crypto-browserify, and to do that we need to move to solana's new v2 webjs library. It's currently in RC so let's keep an eye on that.

https://github.com/solana-labs/solana-web3.js/releases

For now I'll ignore this particular CVE. Thanks!

@kdenhartog
Copy link
Member

Ok, if only solana's web3.js is using crypto-browersify I'm confident this doesn't affect us.

Copy link

[puLL-Merge] - brave-intl/publishers@4487

Description

This PR introduces significant changes to the project structure, test setup, and dependencies. It primarily focuses on updating the testing framework, moving from Capybara to a custom system test setup using Selenium WebDriver with Chromium. The changes also include updates to various gem versions and the introduction of a Next.js testing environment.

Possible Issues

  1. The removal of Firefox-specific test setups may impact browser compatibility testing.
  2. Changes to the test structure and helpers might require updates to existing test cases not included in this PR.
  3. The introduction of a new Next.js testing setup might require additional configuration or environment setup for developers.

Security Hotspots

  1. The addition of a self-signed certificate generation in the create-local-server.js script could potentially weaken the security of the development environment if not properly managed.
Changes

Changes

  1. .audit-ci.json:

    • Added new exceptions for security vulnerabilities related to elliptic curve cryptography.
  2. .github/workflows/test.yml:

    • Removed Firefox setup.
    • Added Next.js installation and build steps.
    • Updated the test running command to use a new script.
  3. Gemfile.lock:

    • Updated various gem versions, including security-related updates.
  4. bin/run_tests.sh:

    • New script to run Next.js server in test mode and execute specific test suites.
  5. nextjs/package.json:

    • Added selfsigned package as a dev dependency.
  6. nextjs/scripts/create-local-server.js:

    • Implemented self-signed certificate generation for development.
    • Added support for test mode configuration.
  7. nextjs/src/app/[locale]/publishers/home/channels/AddChannelModal.tsx:

    • Added id attributes to channel elements for testing purposes.
  8. nextjs/src/app/[locale]/publishers/home/channels/ChannelCard.tsx:

    • Added id attributes to elements for testing purposes.
  9. nextjs/src/app/[locale]/publishers/home/page.tsx:

    • Added id attribute to the "Add Channel" button.
  10. nextjs/src/components/Card.tsx:

    • Added optional id prop to the Card component.
  11. package.json:

    • Updated axios version.
  12. Test files:

    • Moved and renamed various test files from test/features/ to test/system/.
    • Updated test cases to use the new system test setup.
    • Introduced ApplicationSystemTestCase as the base class for system tests.
  13. test/test_helper.rb:

    • Removed Capybara-specific configurations.
    • Added new test helpers and configurations.
  14. New files:

    • test/application_system_test_case.rb: New base class for system tests.
    • test/test_helpers/nextjs_test_setup.rb: Helper module for Next.js test setup.
    • test/test_helpers/sign_in_helpers.rb: Helper module for sign-in functionality in tests.

These changes represent a significant shift in the testing approach, moving towards a more integrated system test setup that includes Next.js components. The PR also includes various dependency updates and security-related changes.

@tsmartt tsmartt merged commit cf7d62f into staging Aug 12, 2024
6 of 7 checks passed
@tsmartt tsmartt deleted the feat/capybara-testing-nextjs branch August 12, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants