Update-Acceptance-highload-edit-form-menu-test Acceptance #1617
Conversation
|
WalkthroughThe changes in the pull request focus on enhancing the structure and logic of the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
tests/acceptance/components/highload-edit-form-menu-test.js (3)
1-8: Consider organizing imports for better readability.Consider grouping imports into three distinct sections:
- Third-party packages (jquery, qunit)
- Ember core imports (@ember/*)
- Local imports (helpers)
import $ from 'jquery'; import { module, test } from 'qunit'; -import startApp from '../../helpers/start-app'; -import { settled } from '@ember/test-helpers'; -import { visit, currentURL } from '@ember/test-helpers'; -import { setupApplicationTest } from 'ember-qunit'; -import { run } from '@ember/runloop'; +import { run } from '@ember/runloop'; +import { settled, visit, currentURL } from '@ember/test-helpers'; +import { setupApplicationTest } from 'ember-qunit'; +import startApp from '../../helpers/start-app';
25-27: Consider extracting path to a constant.The hardcoded path could be moved to a constant at the top of the file for better maintainability.
+const TEST_ROUTE = 'components-examples/highload-edit-form-menu/index'; + test('it properly renders', async function (assert) { assert.expect(7); - let path = 'components-examples/highload-edit-form-menu/index'; - await visit(path); - assert.equal(currentURL(), path); + await visit(TEST_ROUTE); + assert.equal(currentURL(), TEST_ROUTE);
22-54: Consider restructuring test for better maintainability.The test could benefit from several architectural improvements:
- Extract common selectors into a constants object
- Group related assertions into separate test cases
- Consider creating page object for encapsulating UI interactions
- Add comments explaining the business logic being tested
This would make the test more maintainable and easier to understand.
| run(() => { | ||
| $('.tabsNavigation')[0].click(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace run() and jQuery clicks with modern click helper.
The test uses run() and jQuery for click events. Consider using the modern @ember/test-helpers click helper.
- run(() => {
- $('.tabsNavigation')[0].click();
- });
+ await click('[data-test-tab-navigation="0"]');
- run(() => {
- $('.tabsNavigation')[1].click();
- });
+ await click('[data-test-tab-navigation="1"]');
- run(() => {
- $('.showAllFormsButton').click();
- });
+ await click('[data-test-show-all-forms]');Also applies to: 42-44, 49-51
| module('Acceptance | high-edit-form-menu', function (hooks) { | ||
| setupApplicationTest(hooks); | ||
|
|
||
| module('Acceptance | high-edit-form-menu', { | ||
| beforeEach() { | ||
| hooks.beforeEach(function () { | ||
| app = startApp(); | ||
| }, | ||
| }); | ||
|
|
||
| afterEach() { | ||
| hooks.afterEach(function () { | ||
| // Destroy application. | ||
| run(app, 'destroy'); | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider modernizing hooks implementation.
The hooks implementation could be improved for consistency with the async pattern used in the test.
module('Acceptance | high-edit-form-menu', function (hooks) {
setupApplicationTest(hooks);
- hooks.beforeEach(function () {
+ hooks.beforeEach(async function () {
app = startApp();
});
- hooks.afterEach(function () {
+ hooks.afterEach(async function () {
// Destroy application.
- run(app, 'destroy');
+ await run(app, 'destroy');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| module('Acceptance | high-edit-form-menu', function (hooks) { | |
| setupApplicationTest(hooks); | |
| module('Acceptance | high-edit-form-menu', { | |
| beforeEach() { | |
| hooks.beforeEach(function () { | |
| app = startApp(); | |
| }, | |
| }); | |
| afterEach() { | |
| hooks.afterEach(function () { | |
| // Destroy application. | |
| run(app, 'destroy'); | |
| }, | |
| }); | |
| }); | |
| module('Acceptance | high-edit-form-menu', function (hooks) { | |
| setupApplicationTest(hooks); | |
| hooks.beforeEach(async function () { | |
| app = startApp(); | |
| }); | |
| hooks.afterEach(async function () { | |
| // Destroy application. | |
| await run(app, 'destroy'); | |
| }); |
| test('it properly renders', async function (assert) { | ||
| assert.expect(7); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using data-test- selectors.*
Using class selectors for testing is brittle as they may change for styling purposes. Consider adding dedicated test selectors.
- $('.object-list-view').find('tr')[1].children[1].click();
+ await click('[data-test-object-list-row="1"] [data-test-cell="1"]');Committable suggestion was skipped due to low confidence.
| await settled(); | ||
| assert.equal($('.gruppaPolejVvoda').length, 4, 'all tabs are here'); | ||
| assert.equal($('.gruppaPolejVvoda.active').length, 1, 'only one tab is active'); | ||
| assert.equal($('.gruppaPolejVvoda')[0].classList.contains('active'), true, 'first tab is active'); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace jQuery assertions with modern test helpers.
The test relies heavily on jQuery for assertions. Consider using modern test helpers and data-test-* selectors.
- assert.equal($('.gruppaPolejVvoda').length, 4, 'all tabs are here');
- assert.equal($('.gruppaPolejVvoda.active').length, 1, 'only one tab is active');
- assert.equal($('.gruppaPolejVvoda')[0].classList.contains('active'), true, 'first tab is active');
+ assert.dom('[data-test-tab]').exists({ count: 4 }, 'all tabs are here');
+ assert.dom('[data-test-tab].active').exists({ count: 1 }, 'only one tab is active');
+ assert.dom('[data-test-tab="0"]').hasClass('active', 'first tab is active');Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit