-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Media fields: Add tests for fields with unique logic #74015
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
Conversation
| expect( filesizeField ).toMatchObject( { | ||
| id: 'filesize', | ||
| type: 'text', | ||
| label: 'File size', | ||
| enableSorting: false, | ||
| filterBy: false, | ||
| readOnly: true, | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These kind of expectations might be an overkill?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm... could be! If pressed, I'd say the has correct field configuration test does feel a bit overkill as it's unlikely to be a useful test for us. That said, it's very easy to change if it causes any trouble, so I don't mind either way.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @Copilot. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds comprehensive unit tests for media fields with unique logic, covering filename, filesize, and media_dimensions fields. The tests validate field configuration, data extraction/formatting logic, visibility conditions, and view component rendering behavior.
Key Changes:
- Added 7 tests for filename field covering configuration, getValue extraction, and view rendering with various filename lengths
- Added 20 tests for filesize field covering configuration, byte unit formatting (B through TB), and isVisible behavior
- Added 8 tests for media_dimensions field covering configuration, dimension formatting, and isVisible behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/media-fields/src/media_dimensions/test/index.test.ts | Adds tests for media dimensions field configuration, getValue dimension formatting with × separator, and isVisible logic |
| packages/media-fields/src/filesize/test/index.test.tsx | Adds tests for filesize field configuration, byte formatting across multiple units with decimal handling, and isVisible behavior |
| packages/media-fields/src/filename/test/view.test.tsx | Adds tests for filename view component rendering with short/long filenames and edge cases |
| packages/media-fields/src/filename/test/index.test.ts | Adds tests for filename field configuration and getValue extraction from source_url |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
LOL, thanks for all the PRs copilot |
|
Flaky tests detected in 13ae070. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20254574974
|
andrewserong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the tests! Hope you don't mind, but I've pushed a Claude-assisted change to consolidate some of the repetitive tests to use it.each() to group them together for readability. But feel free to keep moving things around if you preferred a different structure.
LGTM! 🚀
| expect( filesizeField ).toMatchObject( { | ||
| id: 'filesize', | ||
| type: 'text', | ||
| label: 'File size', | ||
| enableSorting: false, | ||
| filterBy: false, | ||
| readOnly: true, | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm... could be! If pressed, I'd say the has correct field configuration test does feel a bit overkill as it's unlikely to be a useful test for us. That said, it's very easy to change if it causes any trouble, so I don't mind either way.
Thank you! All changes are welcome 🙇🏻 |
Co-authored-by: ramonjd <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]>
…74021) Co-authored-by: ramonjd <[email protected]>
…74022) Co-authored-by: ramonjd <[email protected]>
…74023) Co-authored-by: ramonjd <[email protected]>
…#74018) Co-authored-by: ramonjd <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: ramonjd <[email protected]>
2ab6faf to
5175d8d
Compare
|
Just rebasing and kicking off tests again. Some funky e2e fails, not related I presume |
|
Oh, I think I might have missed a TypeScript error, I'll take a quick look |
|
Just kicked the e2es off again. Looks like the TS issue is fixed, so I'm pretty sure this one was just flaky. We'll see if the auto-merge winds up doing its thing! |
What?
Related to #72612 (comment) and partial follow up to #73071
Add unit tests for media fields: filename, filesize, media dimensions.
getValue, and view component rendering (short/long filenames, edge cases)isVisiblebehaviorisVisiblebehaviorI think the media thumbnail could an E2E test? Happy to try to mock that up too if required.
Testing Instructions
Tests should pass!