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

Update rpc-handler and fix tests following beta merge #345

Merged
merged 23 commits into from
Oct 25, 2024

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Oct 21, 2024

Resolves #350
Related to ubiquity/rpc-handler#54
Related to #332

  • updates rpc-handler so that we are now actually using the proxy
  • fixes tests that were failing across three suites
  • Uses rpc-handler to fetch ENS name

Tested locally with anvil
Tested on polygon mainnet
Tested with Cypress

Haven't been able to produce a single UI breaking error or problem in regards to RPC interactions both claiming and invalidating but I've watched the retry and proxy logic in action by setting strictLogs: false and watching every log taking place from within the rpc-handler.

I thought it would be easier to just hardcode the permitConfig that you previously fetched with Cypress.env, I think this means that we'll need to re-use one of the permits from one of the prior two tests to use as the "claimed" one? Or something to that effect right?

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 21, 2024

Copy link
Contributor

github-actions bot commented Oct 21, 2024

Preview Deployment
d430ef486ab36e901f0437cfb9b8f2a114d6bfcd

@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 21, 2024

I also investigated why blockscan was giving us 404 errors and I was correct in that it is just not as quick to populate a transaction than a dedicated network explorer is.

In my testing on Polygon it pulled two explorers, the first being polygonscan.com, the official one and I could view the tx instantly. I copied the same hash into blockscan and it would take between 1 and 8 minutes to populate on that platform.

For this reason I've added a toast in the case we can't find an explorer for a given network that gives feedback saying the default generic explorer may take longer to find the tx.

@Keyrxng Keyrxng marked this pull request as ready for review October 21, 2024 21:06
@Keyrxng Keyrxng requested review from 0x4007, EresDev and rndquu and removed request for 0x4007 October 21, 2024 21:06
@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 21, 2024

The tests have been failing consistently for the past two weeks, this PR fixes them.

It would also be good to ensure that tests are ran as part of CI as they haven't been for a little while and it's been allowing PRs through that are breaking tests although I'm not sure how to resolve that off the top of my head.

https://github.com/ubiquity/pay.ubq.fi/blob/development/.github/workflows/cypress-testing.yml - Is it because pull_target is missing?

@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 21, 2024

QA of the tests now working:

https://github.com/ubq-testing/pay.ubq.fi/actions/runs/11449624381/job/31855559926

Ready for review


Not sure of more elegant ways to link this workflow to CI, any suggestions?

  • Why do we wait for the build workflow before running cypress? Can it be ignored and we just run it on pull_request? Unless I'm missing something it's not actually required to run the test suite is it?

Copy link
Contributor

@EresDev EresDev left a comment

Choose a reason for hiding this comment

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

I think this means that we'll need to re-use one of the permits from one of the prior two tests to use as the "claimed" one? Or something to that effect right?

This is problematic. This means one test has a dependency on other test. Tests should be independent. You can move both to same test if test isn't too big. Or do the claiming first in second test.

Or you can reuse one of the previously claimed permit on network 100 as this is what anvil uses by default. Probably will have to deal with signer problem though if you face one.

.github/workflows/cypress-testing.yml Show resolved Hide resolved
cypress/scripts/anvil.ts Show resolved Hide resolved
@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 22, 2024

I think this means that we'll need to re-use one of the permits from one of the prior two tests to use as the "claimed" one? Or something to that effect right?

This is problematic. This means one test has a dependency on other test. Tests should be independent. You can move both to same test if test isn't too big. Or do the claiming first in second test.

Or you can reuse one of the previously claimed permit on network 100 as this is what anvil uses by default. Probably will have to deal with signer problem though if you face one.

I create a permit with a function that uses cy.wrap to hoist it, the 3rd test needs a claimed permit to work and the previous tests create and claim a permit using the url that cy.wrap saves, this is no good?

I assumed you were using a real claimed permit url but I wasn't sure because obv as you say we'd hit a signer issue

@EresDev
Copy link
Contributor

EresDev commented Oct 23, 2024

I create a permit with a function that uses cy.wrap to hoist it, the 3rd test needs a claimed permit to work and the previous tests create and claim a permit using the url that cy.wrap saves, this is no good?

This makes a precondition that test-1 must pass before test-2.
For me it is one of the core rules that should not be broken. One test that shouldn't depend on the other test. And it is just not about dependency. It breaks 2 more rules. Test should be isolated and deterministic.

The best solution is to find an old claimed permit and or just make one even of 0.00001 wxdai and let the test use it. The test will be decoupled, isolated, and deterministic.

@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 23, 2024

I create a permit with a function that uses cy.wrap to hoist it, the 3rd test needs a claimed permit to work and the previous tests create and claim a permit using the url that cy.wrap saves, this is no good?

This makes a precondition that test-1 must pass before test-2. For me it is one of the core rules that should not be broken. One test that shouldn't depend on the other test. And it is just not about dependency. It breaks 2 more rules. Test should be isolated and deterministic.

The best solution is to find an old claimed permit and or just make one even of 0.00001 wxdai and let the test use it. The test will be decoupled, isolated, and deterministic.

I hear you. I'm sure you authored those tests but you were pulling a env variables from your .env that that as far as I could tell you didn't detail or anything which was was I asked for your input initially to understand what conditions you had set that were no longer true. I take it the permit URL you used I cannot use again?

@EresDev
Copy link
Contributor

EresDev commented Oct 23, 2024

I hear you. I'm sure you authored those tests but you were pulling a env variables from your .env that that as far as I could tell you didn't detail or anything which was was I asked for your input initially to understand what conditions you had set that were no longer true. I take it the permit URL you used I cannot use again?

If you are referring to test "should reveal a redeem code after claim", yes you can use the existing permit URL that was there.

@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 23, 2024

I hear you. I'm sure you authored those tests but you were pulling a env variables from your .env that that as far as I could tell you didn't detail or anything which was was I asked for your input initially to understand what conditions you had set that were no longer true. I take it the permit URL you used I cannot use again?

If you are referring to test "should reveal a redeem code after claim", yes you can use the existing permit URL that was there.

Sorry your message read like I'd need to create a new one against the anvil default account and because of the .env thing I thought you were using one of your own testing wallets.

Will address these things today, ty

@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 23, 2024

Hardcoded the permit as requested but I'm still using the createPermitUrl function and piggybacking the callback because Cypress is complaining that clearAllCookies is producing a promise that cy.visit is being invoked inside (when clearAllCookies is a beforeEach fn).

I stabilized the tests in this repo but I'm no cypress or testing/automation expert by any means, is this acceptable?

Kept my anvil spawner logic as you agreed it's the better approach when considering CI.

QA: https://github.com/ubq-testing/pay.ubq.fi/actions/runs/11487208662

@Keyrxng Keyrxng requested a review from EresDev October 23, 2024 20:01
@rndquu rndquu requested review from rndquu and removed request for rndquu October 24, 2024 14:46
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

  1. Local claims seems to be working fine
  2. Hope this PR adds more stability
  3. I really need it for reloadly gift cards production setup

@EresDev
Copy link
Contributor

EresDev commented Oct 24, 2024

I gave it a run. Rpc is working nicely for me. However, there are some problems with the cypress tests.

  1. The tests are not passing in the given QA, see their logs. The tests are failing because reloadly sandbox keys were not added to the source repository. They are e2e tests and need reloadly sandbox keys (ask me if you don't have one).
    RELOADLY_SANDBOX_API_CLIENT_ID: ${{ secrets.RELOADLY_SANDBOX_API_CLIENT_ID }}

    At least some tests should report failure. This needs fix along with the addition of sandbox keys.
  2. The claimUrl is same in 2 test files.
    image
    Even though this is not ideal, but currently the blockchain is shared in tests. When one test claims one permit, the second test will fail. Both need different claim URL.

@Keyrxng
Copy link
Member Author

Keyrxng commented Oct 24, 2024

https://github.com/ubq-testing/pay.ubq.fi/actions/runs/11487208662

  1. My wf run is fully passing. I see what you mean re: reloadly but these are tests you wrote bud I intentionally made as few changes to those as possible, assuming you had written them to cover your feature effectively. I do have keys that's not a problem, are the proper ones defined in this repo as that's where it really counts?

  2. https://github.com/ubq-testing/pay.ubq.fi/blob/development/cypress/e2e/claim-portal-non-web3.cy.ts does not have tests that rely on the status of the permitURL. It is simply there so it loads the UI properly, it has no effect on tests in that suite otherwise. Can change it but it's a no-op either way.


- name: Cypress run
uses: cypress-io/github-action@v6
with:
build: yarn run build
start: yarn test:fund
start: npx wrangler pages dev static --port 8080 --binding USE_RELOADLY_SANDBOX=true RELOADLY_API_CLIENT_ID="$RELOADLY_SANDBOX_API_CLIENT_ID" RELOADLY_API_CLIENT_SECRET="$RELOADLY_SANDBOX_API_CLIENT_SECRET" &
Copy link
Member Author

Choose a reason for hiding this comment

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

@EresDev you had this as it's own step before but that's not right as the env hadn't fully setup yet that's why I've made this change here.

Is this the correct way to pass reloadly env vars?

Copy link
Member Author

@Keyrxng Keyrxng Oct 24, 2024

Choose a reason for hiding this comment

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

https://github.com/ubq-testing/pay.ubq.fi/actions/runs/11504344113/job/32023622908#step:7:1

I just added the id and secret for the sandbox taken directly from the google doc to my repo secrets and the errors are still present.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is logic missing inside the cards feature that is not throwing the error properly otherwise Cypress would fail, do you agree?

The log style looks unfamiliar to me also so I'm not 100% where it's coming from, so I think it needs it's own task and shouldn't block this pr.

Copy link
Contributor

@EresDev EresDev left a comment

Choose a reason for hiding this comment

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

My wf run is fully passing.

Yes, tests are passing now. I ran multiple times, and one test was failing that time continuously. For me the quick guess was the claim URL. But I see that is what e2e tests do sometimes. They seem fine now.

Is this the correct way to pass reloadly env vars?

Seems correct, but I am also learning these workflow. So, no expert here either.

I just added the id and secret for the sandbox taken directly from the google doc to my repo secrets and the errors are still present.

Yes, I just tried myself too, and they are still showing up.

I think there is logic missing inside the cards feature that is not throwing the error properly otherwise Cypress would fail, do you agree?

I feel like it is a problem with workflow code (how they are passing those secrets) and probably not code side. Code works fine on my local setup. I will look more into this with my next PR.

@rndquu rndquu merged commit d430ef4 into ubiquity:development Oct 25, 2024
3 checks 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.

Stabilize broken test suite
3 participants