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

refactor: cleanup preview items on drops page #10344

Merged
merged 12 commits into from
Jun 6, 2024

Conversation

preschian
Copy link
Member

@preschian preschian commented May 24, 2024

Thank you for your contribution to the Koda - Generative Art Marketplace.

👇 __ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

(please remove design and QA checks below if not needed)

Needs Design check

Needs QA check

  • @kodadot/qa-guild please review

Context

Did your issue had any of the "$" label on it?

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

Copy link

netlify bot commented May 24, 2024

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit e28f564
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/665f3372aeed2d0008538ad5
😎 Deploy Preview https://deploy-preview-10344--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

cloudflare-pages bot commented May 24, 2024

Deploying koda-art-prod with  Cloudflare Pages  Cloudflare Pages

Latest commit: e28f564
Status: ✅  Deploy successful!
Preview URL: https://430211a8.kodaart-production.pages.dev
Branch Preview URL: https://refactor--cleanup-preview-dr.kodaart-production.pages.dev

View logs

@exezbcz
Copy link
Member

exezbcz commented May 27, 2024

linking:

@preschian preschian marked this pull request as ready for review May 28, 2024 01:45
@preschian preschian requested a review from a team as a code owner May 28, 2024 01:45
@preschian preschian requested review from daiagi and Jarsen136 and removed request for a team May 28, 2024 01:45
* refactor(e2e): modify nft-name test to check the first element in collection.spec.ts
… TIMEOUT

* feat(playwright.config.ts): add expect timeout setting
* refactor(tests/e2e/collection.spec.ts): remove redundant waitForLoadState
* refactor(tests/e2e/collection.spec.ts): simplify nft-name text expectation
@preschian preschian requested a review from hassnian May 28, 2024 03:18
components/collection/drop/modal/PaidMint.vue Show resolved Hide resolved
tests/randomize.spec.ts Outdated Show resolved Hide resolved
@prury
Copy link
Member

prury commented May 28, 2024

did some mints using auto-teleport on canary and was unable to reproduce the issue, any ideas?

…g in CI environment

* refactor(explore.spec.ts, sidebar.spec.ts): remove explicit timeout from toBeVisible assertions
@prury
Copy link
Member

prury commented May 29, 2024

@preschian sidebar.spec.ts is failing on other PRs as well, investigating.

@preschian
Copy link
Member Author

sidebar.spec.ts is failing on other PRs as well, investigating.

I tried to increase the timeout in the playwright global config. Should we increase it again? currently 12s. On local, sidebar.spec.ts looks fine

@prury
Copy link
Member

prury commented May 29, 2024

sidebar.spec.ts is failing on other PRs as well, investigating.

I tried to increase the timeout in the playwright global config. Should we increase it again? currently 12s. On local, sidebar.spec.ts looks fine

uh, not sure if i understood
from what I've seen you have increased the global test timeout from 1 min to 2 min (1 * 60 * 1000 to 2 * 60 * 1000), and retries from 1 to 3

i would leave the global test timeout at 1 min, meaning that, if a single test takes more than one minute, it would automatically fail(i try to keep each test below 1min)

retries i would also leave at 1, otherwise the test will take a long time to fail (imagine if a test takes 50s and it retries it 3 times)

if you were willing to change the expect timeout(5s default) we have to use: config = { expect: { timeout: 10000 } }

i think sidebar.spec.ts is failing for another yet unknown reason

@preschian
Copy link
Member Author

from what I've seen you have increased the global test timeout from 1 min to 2 min (1 * 60 * 1000 to 2 * 60 * 1000), and retries from 1 to 3

Oh sorry, yes this one correct. Not 12s but to 2 min

if you were willing to change the expect timeout(5s default) we have to use: config = { expect: { timeout: 10000 } }

Yes, this is the one I add. Right now, the config:

const TIMEOUT = 2 * 60 * 1000

const config = {
	// ...
	timeout: TIMEOUT,
	expect: {
	  timeout: TIMEOUT,
	},
	// ...
}

playwright.config.ts Outdated Show resolved Hide resolved
@prury
Copy link
Member

prury commented May 29, 2024

as i said in dm, tests were failing due to an RPC error, that's why it failed across many PRs
this RPC error prevented the app from getting balances, making the add funds button never to reach load state on an empty account

feel free to revert config or timeout changes in tests, or test the 12s global timeout

@vikiival vikiival requested review from Jarsen136 and prury June 3, 2024 10:19
Copy link
Contributor

@hassnian hassnian left a comment

Choose a reason for hiding this comment

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

the only thing is that now we won't be able to see NFT previews while mass-minting , if @exezbcz approves let's go

@exezbcz
Copy link
Member

exezbcz commented Jun 4, 2024

hmm weird, the modal does not even load for me
image

image

@exezbcz
Copy link
Member

exezbcz commented Jun 4, 2024

@hassnian that is the thing, we dont show any previews because what you get is random now

@prury prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Jun 4, 2024
@preschian
Copy link
Member Author

hmm weird, the modal does not even load for me

let me check

Copy link

codeclimate bot commented Jun 4, 2024

Code Climate has analyzed commit e28f564 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link

sonarcloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@preschian
Copy link
Member Author

hmm weird, the modal does not even load for me

interesting, how about now @exezbcz , probably due to error on rpc side

@prury prury removed the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Jun 4, 2024
@prury prury added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label Jun 4, 2024
@prury prury added this pull request to the merge queue Jun 6, 2024
Merged via the queue into main with commit 97fcbe1 Jun 6, 2024
20 checks passed
@prury prury deleted the refactor--cleanup-preview-drop-items branch June 6, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-visual-ok-✅ S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants