feat: proposal e2e approach#354
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are several critical issues that will break the e2e flows and violate mandatory checklist items. The most severe problem is in cypress/support/commands.js: custom commands like getByDataCy, register, and login call cy.get(...) / cy.request(...) but do not return those chains, which means every Page Object getter that does return cy.getByDataCy(...) will actually return undefined and calls like .click(), .clear(), or .type() in files such as settings.pageObject.js, profile.pageObject.js, signUp.pageObject.js, and articleEdit.pageObject.js will fail at runtime; you should update each command to return the Cypress chainable to restore proper chaining. In src/vue/store/actions.js, the fetchProfile action’s catch block uses a parameter named response but then logs error.response, which will throw a ReferenceError on failures, and some actions (e.g. toggleArticleFavorite, fetchProfile) never resolve their Promise on the success path; these need to be fixed so that both success and error paths resolve/reject consistently and profile/favorite flows work reliably. Also in actions.js, setFollowProfile currently ignores its params and simply toggles profile.following with !profile.following; this should honor an explicit params.value when provided (and use params.username where appropriate) so follow/unfollow behaves deterministically for the tests. For the Cypress checklist, DB cleanup is currently done only in before() in suites like cypress/e2e/signIn.cy.js and signUp.cy.js; per [CHECKLIST ITEM #24] the database must be cleared before each test, so move cy.task('db:clear') into a beforeEach() or call it at the start of every it() to ensure test isolation. The negative auth tests (e.g. in cypress/e2e/signIn.cy.js and the Sign Up negative case) still use hard-coded invalid email/password values; per [CHECKLIST ITEM #14] replace those with cy.task('generateUser') or faker-based data so that invalid credentials are generated rather than static. Finally, some assertions still rely on implementation-specific CSS selectors like .swal-modal / .swal-title and .article-page h1; to satisfy [CHECKLIST ITEM #25] you should add data-cy (or data-qa) attributes to these elements in the Vue components and update the tests and Page Objects to use those stable selectors instead. Once these command chainability issues, Vuex action bugs, and checklist violations are addressed, your existing PageObject-based tests and flows should run much more reliably, and the overall structure you already have in place will serve you well.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| 'src/**', | ||
| 'console/**', | ||
| 'server/**', | ||
| 'node_modules/**', |
There was a problem hiding this comment.
The test suite clears the DB in before() (see file-level setup) which runs once for the file. To ensure test isolation and satisfy checklist item #24, consider using beforeEach() or calling cy.task('db:clear') before each test so each it() starts from a clean DB state.
| 'server/**', | ||
| 'node_modules/**', | ||
| 'webpack.config.js', | ||
| 'cypress/support/index.d.ts' |
There was a problem hiding this comment.
The negative test types a hard-coded email. Per checklist item #14 (use faker/custom methods to generate fake data), use cy.task('generateUser') or faker to create a random email for this test instead of a static value.
| 'node_modules/**', | ||
| 'webpack.config.js', | ||
| 'cypress/support/index.d.ts' | ||
| ], |
There was a problem hiding this comment.
The negative test types a hard-coded password. Use generated/faker data for passwords as well (see checklist item #14) to avoid brittle static test data.
| 'webpack.config.js', | ||
| 'cypress/support/index.d.ts' | ||
| ], | ||
| rules: { |
There was a problem hiding this comment.
After clicking sign-in you rely on the swal error modal — consider adding an explicit wait/assert for the request/response or checking the UI state (e.g. header not showing username) to make the negative assertion more robust.
| ], | ||
| rules: { | ||
| semi: ['error', 'always'], | ||
| 'space-before-function-paren': 'off', |
There was a problem hiding this comment.
The test asserts on swal elements using CSS classes ('.swal-modal', '.swal-title'). To follow checklist item #25 (use data-qa/data-cy attributes for elements tested), add data-cy attributes to the swal modal/title or create helper selectors so tests don't rely on implementation-specific class names.
| }); | ||
|
|
||
| it('should not provide an ability to log in with wrong credentials', () => { | ||
| signInPage.visit(); |
There was a problem hiding this comment.
Tests clear the DB only once (in the suite setup). The task requires clearing all data before the test (checklist item #24). Consider moving DB cleanup to run before each test (use beforeEach or call cy.task('db:clear') at the start of each it-block) to ensure test isolation.
| it('should not provide an ability to log in with wrong credentials', () => { | ||
| signInPage.visit(); | ||
|
|
||
| signInPage.typeEmail('wrong@email.com'); |
There was a problem hiding this comment.
The negative login test uses hard-coded credentials. The checklist requires using faker / custom methods to generate fake data (checklist item #14). Use cy.task or faker to generate a random invalid email/password so the test doesn't rely on static values.
| import SignUpPageObject from '../support/pages/signUp.pageObject'; | ||
| import HomePageObject from '../support/pages/home.pageObject'; | ||
|
|
||
| const signUpPage = new SignUpPageObject(); |
There was a problem hiding this comment.
The declarations indicate these commands return Chainable<any>, but the implementations in cypress/support/commands.js don't return the chainables (they call cy.get / cy.request without returning). Consider returning the cy.* calls in the command implementations so the runtime behaviour matches these typings and enables proper chaining in TypeScript.
| interface Chainable<Subject> { | ||
| getByDataCy(selector: string): Chainable<any> | ||
| register(email: string, username: string, password: string): Chainable<any> | ||
| login(email: string, password: string): Chainable<any> |
There was a problem hiding this comment.
Keep the TypeScript declarations in sync: the Chainable signatures assume these commands return Chainable. After fixing the custom commands to return their cy chains, the index.d.ts signatures will correctly reflect actual behavior.
|
|
||
| class ArticleEditPageObject extends PageObject { | ||
| url = '/#/editor'; | ||
|
|
There was a problem hiding this comment.
This getter returns cy.getByDataCy('edit-article-btn'). Ensure the custom getByDataCy command returns the Cypress chainable (return cy.get(...)); otherwise this getter will return undefined and click() will fail.
| get titleField() { | ||
| return cy.getByDataCy('title-input'); | ||
| } | ||
|
|
There was a problem hiding this comment.
This getter returns cy.getByDataCy('delete-article-btn'). It depends on getByDataCy returning the chainable; confirm that command returns the cy.get(...) result so this works reliably.
| get publishBtn() { | ||
| return cy.getByDataCy('publish-btn'); | ||
| } | ||
|
|
There was a problem hiding this comment.
The assertion uses a CSS selector .article-page h1. To follow the checklist item that requires stable data-* attributes for test-targeted elements, add and use a data-cy (or data-qa) attribute on the article title element and use getByDataCy here instead.
| url = '/#/editor'; | ||
|
|
||
| get titleField() { | ||
| return cy.getByDataCy('title-input'); |
There was a problem hiding this comment.
This getter uses cy.getByDataCy('title-input'). As above, ensure getByDataCy returns the chainable so methods like typeTitle (which call .clear().type(...)) operate on a valid element.
| } | ||
|
|
||
| get bodyField() { | ||
| return cy.getByDataCy('body-input'); |
There was a problem hiding this comment.
The bodyField getter uses cy.getByDataCy('body-input'). Confirm getByDataCy returns the chainable; otherwise .clear().type(...) in typeBody will error at runtime.
|
|
||
| get descriptionField() { | ||
| return cy.getByDataCy('description-input'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Profile page getters rely on cy.getByDataCy('follow-btn')/unfollow-btn. Make sure getByDataCy returns the Cypress chainable so visibility assertions and clicks work correctly.
| return cy.getByDataCy('title-input'); | ||
| } | ||
|
|
||
| get descriptionField() { | ||
| return cy.getByDataCy('description-input'); | ||
| } | ||
|
|
||
| get bodyField() { | ||
| return cy.getByDataCy('body-input'); | ||
| } | ||
|
|
||
| get publishBtn() { | ||
| return cy.getByDataCy('publish-btn'); | ||
| } | ||
|
|
||
| typeTitle(title) { | ||
| this.titleField.clear().type(title); | ||
| } | ||
|
|
||
| typeDescription(description) { | ||
| this.descriptionField.clear().type(description); |
There was a problem hiding this comment.
Settings page getters (username, bio, email, password, update/logout buttons) use getByDataCy. Verify getByDataCy returns the cy.get(...) result so the type... and click... methods work and tests do not fail due to undefined returns.
| return cy.getByDataCy('username-settings'); | ||
| } | ||
|
|
||
| get bioField() { | ||
| return cy.getByDataCy('bio-settings'); | ||
| } | ||
|
|
||
| get emailField() { | ||
| return cy.getByDataCy('email-settings'); | ||
| } | ||
|
|
||
| get passwordField() { | ||
| return cy.getByDataCy('password-settings'); | ||
| } | ||
|
|
||
| get updateSettingsBtn() { | ||
| return cy.getByDataCy('update-settings-btn'); | ||
| } | ||
|
|
||
| get logoutBtn() { | ||
| return cy.getByDataCy('logout-btn'); |
There was a problem hiding this comment.
These getters return cy.getByDataCy(...). The custom command getByDataCy currently does not return the cy chain, so these getters will return undefined and calls like .clear() / .type() will fail. Fix options: (1) change cypress/support/commands.js to Cypress.Commands.add('getByDataCy', selector => { return cy.get(...); }), or (2) change each getter here to return cy.get([data-cy="..."]) directly. This is a critical fix to restore chainability.
| clickUpdateSettingsBtn() { | ||
| this.updateSettingsBtn.click(); | ||
| } | ||
|
|
||
| clickLogoutBtn() { | ||
| this.logoutBtn.click(); |
There was a problem hiding this comment.
The click helper methods call this.updateSettingsBtn.click() and this.logoutBtn.click() but do not return the chainable. It's preferable to return this.updateSettingsBtn.click() (and similarly for logout) so callers can chain assertions or waits on the click result.
|
|
||
| class ProfilePageObject extends PageObject { | ||
| visit(username) { | ||
| cy.visit(`/#/@${username}`); |
There was a problem hiding this comment.
This getter returns cy.getByDataCy('edit-article-btn'). Currently the custom getByDataCy command does not return the cy chain, so this getter will be undefined at runtime. Either make getByDataCy return cy.get(...) in commands.js or change this getter to return cy.get('[data-cy="edit-article-btn"]') so calls like .click() work.
| } | ||
|
|
||
| get followBtn() { | ||
| return cy.getByDataCy('follow-btn'); |
There was a problem hiding this comment.
Same issue as above for the delete button getter: return cy.getByDataCy('delete-article-btn') will be undefined unless getByDataCy returns the chain. Fix the command or return cy.get(...) here.
| } | ||
|
|
||
| get unfollowBtn() { | ||
| return cy.getByDataCy('unfollow-btn'); |
There was a problem hiding this comment.
This method calls this.editArticleBtn.click(). If the editArticleBtn getter returns undefined (see previous comments), this .click() will throw. Ensure the getter returns a chainable before invoking .click().
| } | ||
|
|
||
| clickUnfollowBtn() { | ||
| this.unfollowBtn.click(); |
There was a problem hiding this comment.
This assertion uses a plain CSS selector cy.get('.article-page h1'). Per checklist item #25, prefer using a stable data-cy/data-qa attribute for elements you assert on. Add a data-cy to the article title in the app and assert via cy.getByDataCy('article-title').
| visit(username) { | ||
| cy.visit(`/#/@${username}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
This getter return cy.getByDataCy('title-input') suffers the same chainability issue: getByDataCy must return the cy.get chain or this should return cy.get('[data-cy="title-input"]') so .clear().type() in typeTitle works.
| get followBtn() { | ||
| return cy.getByDataCy('follow-btn'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same for the description field getter — ensure getByDataCy returns the chain or use cy.get(...) here so subsequent .clear().type() calls succeed.
| get unfollowBtn() { | ||
| return cy.getByDataCy('unfollow-btn'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same for the body field getter — fix chainability to allow .clear().type() in typeBody to work as expected.
| clickUnfollowBtn() { | ||
| this.unfollowBtn.click(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The typeTitle method calls this.titleField.clear().type(title). If titleField doesn't return the chainable element, these commands will fail. Ensure the getter returns the cy chain.
| } | ||
|
|
||
| get followBtn() { | ||
| return cy.getByDataCy('follow-btn'); |
There was a problem hiding this comment.
This getter return cy.getByDataCy('follow-btn') will be undefined unless getByDataCy returns the cy chain. Update the custom command or return cy.get(...) here so .click() and .should() work.
| } | ||
|
|
||
| get unfollowBtn() { | ||
| return cy.getByDataCy('unfollow-btn'); |
There was a problem hiding this comment.
Same for the unfollowBtn getter — ensure the command returns the chain so click() and should() operate correctly.
| } | ||
|
|
||
| assertFollowBtnVisible() { | ||
| this.followBtn.should('be.visible'); |
There was a problem hiding this comment.
The assertFollowBtnVisible uses this.followBtn.should('be.visible'). If followBtn doesn't return the chainable, this assertion will fail. Fix the underlying getter chainability.
|
|
||
| get followBtn() { | ||
| return cy.getByDataCy('follow-btn'); | ||
| } | ||
|
|
||
| get unfollowBtn() { | ||
| return cy.getByDataCy('unfollow-btn'); | ||
| } | ||
|
|
||
| clickFollowBtn() { | ||
| this.followBtn.click(); | ||
| } | ||
|
|
||
| clickUnfollowBtn() { | ||
| this.unfollowBtn.click(); | ||
| } | ||
|
|
||
| assertFollowBtnVisible() { | ||
| this.followBtn.should('be.visible'); | ||
| } | ||
|
|
There was a problem hiding this comment.
The settings page getters (username/bio/email/password/update/logout) all return cy.getByDataCy(...). They will all be undefined unless getByDataCy is updated to return the cy chain. Update the command implementation or change these getters to cy.get('[data-cy="..."]').
| return cy.getByDataCy('username-sign-up'); | ||
| } | ||
|
|
||
| get emailField() { | ||
| return cy.getByDataCy('email-sign-up'); | ||
| } | ||
|
|
||
| get passwordField() { | ||
| return cy.getByDataCy('password-sign-up'); | ||
| } | ||
|
|
||
| get signUpBtn() { | ||
| return cy.getByDataCy('sign-up-btn'); | ||
| } | ||
|
|
||
| get errorMessages() { | ||
| return cy.getByDataCy('error-messages'); |
There was a problem hiding this comment.
Multiple getters return cy.getByDataCy(...). Ensure the custom command getByDataCy returns the Cypress chainable (i.e. return cy.get(...)) or change these getters to return cy.get('[data-cy="..."]') directly. Without that change, calls like .clear().type() or .click() will fail because the getter won't provide a Chainable.
| typeUsername(username) { | ||
| this.usernameField.clear().type(username); | ||
| } | ||
|
|
||
| typeEmail(email) { | ||
| this.emailField.clear().type(email); | ||
| } | ||
|
|
||
| typePassword(password) { | ||
| this.passwordField.clear().type(password); | ||
| } |
There was a problem hiding this comment.
Action methods (e.g. typeUsername, typeEmail, typePassword) call this.usernameField.clear().type(...) but do not return the chainable. Prefer returning the chain (for example return this.usernameField.clear().type(username)) so tests can chain assertions or wait reliably on these actions.
| clickSignUpBtn() { | ||
| this.signUpBtn.click(); |
There was a problem hiding this comment.
The clickSignUpBtn method calls this.signUpBtn.click() but doesn't return it. Return the click chain (e.g. return this.signUpBtn.click()) to allow callers to wait for navigation or subsequent UI changes.
| setFollowProfile(context, params) { | ||
| const profile = { ...context.getters.profile }; | ||
| profile.following = !profile.following; | ||
| context.commit("setProfile", profile); | ||
| }, |
There was a problem hiding this comment.
setFollowProfile currently flips the saved profile's following flag by doing profile.following = !profile.following, ignoring the params argument. Callers (and tests) pass { username, value } into this action — prefer honoring an explicit params.value when provided (e.g. profile.following = typeof params.value === 'boolean' ? params.value : !profile.following) so the action behaves predictably. Also consider persisting this change through the API if required by the app flow.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical issues that can break the app and tests at runtime.
Critical issues that must be fixed before this can be considered complete:
- In
src/vue/routes/Article.vue, theauthorImage()accessor currently returnsarticle.author.imageinstead ofthis.article.author.image, which will throw aReferenceErrorwhen the article page renders and will break article-related flows; update it to usethis.(and optionally add a null-safe guard) so the page renders reliably. - In
server/db.js, theclear()function starts a transaction (const t = await sequilize.transaction()) but never passes that transaction into theDELETEqueries, the delete order can violate foreign-key constraints (e.g. deletingarticlesbeforearticle_comments,usersbeforesessions), and thecatchblock rolls back but does not rethrow or propagate the error; please (a) delete dependent tables first (e.g.article_comments→articles,sessions→users), (b) include{ transaction: t }in each query so commit/rollback actually control them, and (c) rethrow or return a rejected Promise after rollback so the Cypressdb:cleartask fails visibly when cleanup fails. Related helper code incypress/support/pages/signUp.pageObject.jsthat issues rawDELETEstatements should follow the same safe order and pass the transaction object into each query. Also,seed()is exported but empty inserver/db.js(and related modules); either implement seeding as required or remove it from the exports to avoid a misleading API. [CHECKLIST ITEM #24] - In
src/vue/store/actions.js,setFollowProfile(around lines 340–345) ignoresparams.usernameand always mutates the currently stored profile, which can cause non‑deterministic or incorrect follow/unfollow behavior when a different username is dispatched; update this action to explicitly target the profile identified byparams.username(or verify that the current profile matches it) and setprofile.followingdeterministically usingparams.valuewhen provided or the API response instead of relying on a blind toggle.
Smaller improvements (not blocking, but recommended): make createArticleComment follow the same error-handling pattern as other actions (resolve vs reject), return the Promise from logOut so callers can .then() it, guard setArticle (around lines 287–291) against article.tags being undefined, and have the cy.login command return the cy.setCookie(...) chain inside its .then() for more predictable chaining. Overall your test suite, Page Objects, and data-cy usage are in good shape; addressing the issues above will make the backend and Vuex behavior as solid and deterministic as the tests you’ve written.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| }).then((response) => { | ||
| const token = response.body.user.token; | ||
| cy.setCookie('drash_sess', token); |
There was a problem hiding this comment.
Small improvement: inside the login command's .then() callback you call cy.setCookie('drash_sess', token) but don't return it. Returning the cy.setCookie(...) chain (or returning the response) makes the resulting chainable more predictable for callers who may want to chain .then() after cy.login(...).
|
|
||
| get passwordField() { | ||
| return cy.getByDataCy('password-sign-up'); | ||
| } |
There was a problem hiding this comment.
The DELETE statements run here remove articles before article_comments. If article_comments has a foreign key referencing articles, deleting the parent first will fail. Also you start a transaction (const t = await sequilize.transaction()), but the queries below are not executed inside that transaction — pass the transaction to each query, e.g. await sequilize.query('DELETE FROM article_comments;', { transaction: t }), and delete child tables first (article_comments → articles).
| } | ||
|
|
||
| typeUsername(username) { | ||
| return this.usernameField.clear().type(username); |
There was a problem hiding this comment.
You export seed together with clear, but seed() is empty. Either implement seed data (if required by tests) or stop exporting it to avoid confusion. Also consider making clear() surface failures (log or rethrow) so CI can detect when clearing fails.
| await sequilize.query('DELETE FROM articles;'); | ||
| await sequilize.query('DELETE FROM article_comments;'); | ||
| await sequilize.query('DELETE FROM sessions;'); | ||
| await sequilize.query('DELETE FROM users;'); |
There was a problem hiding this comment.
The DELETE queries are executed without using the transaction t you created. Pass the transaction to each query (e.g. await sequilize.query(sql, { transaction: t })) so that commit/rollback control those queries. Also the current delete order can violate foreign-key constraints — delete dependent tables first (e.g. article_comments before articles, and sessions before users).
| } | ||
|
|
||
| module.exports = { clear }; | ||
| module.exports = { clear, seed }; |
There was a problem hiding this comment.
The catch block rolls back the transaction but does not rethrow or otherwise signal the error. That means failures during cleanup will be swallowed and Cypress won't know the task failed. After rollback, rethrow the error (or return a rejected Promise) so the db:clear task can fail the test run. Also note seed() is empty — either implement it or remove/export accordingly.
| <script src="/public/js/app.js"></script> | ||
| <script> | ||
| var observer = new MutationObserver(function(mutations) { | ||
| var title = document.querySelector('.swal-title'); | ||
| if (title && !title.getAttribute('data-cy')) { |
There was a problem hiding this comment.
seed() is exported but has no implementation. If CI/tests expect a db:seed task to provide data, implement it and return a promise; otherwise remove it from the exports to avoid confusion.
| <div id="app"></div> | ||
| <script src="/public/js/app.js"></script> | ||
| <script> |
There was a problem hiding this comment.
The login custom command returns cy.request(...).then(...), but inside the .then() you call cy.setCookie('drash_sess', token) and don't return it. Returning the cy.setCookie(...) chain (or returning the response) will make cy.login() chainable/predictable for callers who want to chain further commands after login.
| setFollowProfile(context, params) { | ||
| const profile = { ...context.getters.profile }; | ||
| profile.following = typeof params.value === 'boolean' | ||
| ? params.value | ||
| : !profile.following; | ||
| context.commit("setProfile", profile); |
There was a problem hiding this comment.
Determinism / API: setFollowProfile currently copies context.getters.profile and toggles profile.following unless params.value is provided. It does not use params.username at all, so if the passed params refers to a different username than the currently stored profile this will silently update the wrong profile. The task requires deterministic follow/unfollow behavior and honoring params.username and params.value. Consider changing this action to: fetch or identify the profile by params.username, then set profile.following to params.value when it's a boolean; otherwise set it explicitly based on the API response (don't rely solely on toggling !profile.following). This will make tests deterministic and align with the checklist.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this submission as an educational override so you can continue progressing while you address a few remaining issues and strengthen your understanding. You’ve done a solid job setting up a comprehensive Cypress suite with PageObjects, faker-based generators, and a transactional server/db.clear() implementation; core flows like article CRUD, profile/settings updates, and authentication are all covered. The most important fixes to tackle next are: (1) standardizing your selectors to use the required data-qa attributes instead of data-cy across components, PageObjects, and helpers (this directly affects the “Add data-qa attributes” requirement [CHECKLIST ITEM #X]); (2) correcting the Vuex setFollowProfile action in src/vue/store/actions.js so it uses params.username and params.value deterministically rather than blindly toggling the current profile; and (3) ensuring auth-related tests don’t start already logged in by creating users without setting the cookie (instead of always using cy.register, which sets drash_sess). In addition, make the DB connection in server/db.js configurable via environment variables (so Docker containers connect via the postgres service name instead of hard-coded localhost:54320), and fix the potentially invalid faker.lorem.words({ min: 2, max: 5 }) usage in cypress.config.js by first generating a numeric count with faker.number.int. For robustness, remove the duplicate mounted() defined inside methods in src/vue/routes/Settings.vue, guard against article.tags being undefined in your Vuex actions and editor route, and normalize tags as string | string[] in your createArticle command and its TypeScript declaration. Overall, your structure, use of PageObjects, and DB-cleaning strategy are strong foundations—treat these remaining issues as practice in tightening correctness and aligning with real-world requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| addMatchImageSnapshotPlugin | ||
| } = require('cypress-image-snapshot/plugin'); | ||
|
|
||
| module.exports = defineConfig({ |
There was a problem hiding this comment.
This line contains nul, which looks like a typo. If you intended to ignore a file named null or a platform-specific file, please correct or remove it. As-is it does nothing meaningful and may confuse readers.
| tags: faker.lorem.words( | ||
| { min: 2, max: 5 } | ||
| ).split(' ') |
There was a problem hiding this comment.
This uses faker.lorem.words({ min: 2, max: 5 }). Depending on the installed @faker-js/faker version, faker.lorem.words() may expect a numeric count rather than an options object and could throw. Consider using a numeric argument (e.g. faker.lorem.words(faker.number.int({ min: 2, max: 5 }))) or the appropriate API for your faker version (for example faker.helpers.arrayElements(...) or faker.word.words(...)) to ensure tags becomes an array of words.
| files: ['**/*.js'], | ||
| extends: ['plugin:cypress/recommended'], | ||
| rules: { | ||
| '@typescript-eslint/no-var-requires': 'off' |
There was a problem hiding this comment.
The CI workflow triggers on pull requests to the main branch (see branch value on the manifest). I noticed other workflows in the repo reference master; please ensure the branch name is consistent across workflows (use the repository's actual default branch) to avoid CI not running when expected.
| ], | ||
| ignorePatterns: [ | ||
| 'src/**', |
There was a problem hiding this comment.
The tags generator uses faker.lorem.words({ min: 2, max: 5 }). In @faker-js/faker the lorem.words() method expects a number (count) rather than an options object in many versions. Passing an object may not behave as intended. Instead, generate a random integer for the count (e.g. const tagCount = faker.number.int({ min: 2, max: 5 })) and then call faker.lorem.words(tagCount), splitting the returned string into an array. This will produce a more reliable tags array.
|
|
||
| - name: Install packages | ||
| run: npm install | ||
|
|
||
| run: npm ci |
There was a problem hiding this comment.
faker.lorem.words(...) usually expects a number argument (count). Passing an options object ({ min: 2, max: 5 }) is not supported by most @faker-js/faker versions and may cause runtime errors. Instead, generate a random count and call faker.lorem.words(count) (for example: const count = faker.number.int({ min: 2, max: 5 }); faker.lorem.words(count).split(' ')).
|
|
||
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| return cy.register( | ||
| user.email, user.username, user.password |
There was a problem hiding this comment.
Registering the user here via cy.register(...) sets the session cookie and logs the test user in. Because the sign-in tests are intended to exercise the login flow (positive and negative), having the user already authenticated will invalidate those tests. Create the user without setting the cookie (e.g. via a server-side cy.task that inserts the user) or clear the cookie before performing the login actions so the tests actually run against the login page.
| articleEditPage.typeDescription(article.description); | ||
| articleEditPage.typeBody(article.body); | ||
| articleEditPage.clickPublishBtn(); | ||
|
|
||
| cy.url().should('include', '/articles/'); | ||
| articlePage.assertArticleTitle(article.title); | ||
| }); | ||
|
|
||
| it('should be edited using Edit button', () => { |
There was a problem hiding this comment.
The negative login test asserts on the presence of an error modal (.swal-title contains 'Login failed'). If the user is already authenticated (see the registration in beforeEach), the app may redirect away from the login page and the expected error will not appear. Ensure the session is cleared before this test so the app actually attempts to log in with the provided (wrong) credentials.
| cy.task('generateArticle').then((generatedArticle) => { | ||
| article = generatedArticle; |
There was a problem hiding this comment.
You call cy.task('generateArticle') separately in beforeEach (not chained to the preceding user registration/registration .then() chain). Cypress preserves command order, but for clarity and to avoid subtle timing issues, consider chaining the article generation after the registration completes or returning the combined chain from beforeEach so the setup is explicitly sequential.
| }); | ||
|
|
||
| it('should provide an ability to log in with existing credentials', () => { | ||
| it('should provide an ability to log in', () => { |
There was a problem hiding this comment.
The test uses the page object form helper (fillForm(user)) which ultimately depends on data-cy attributes (via the page objects). The task description requires data-qa attributes for elements used in tests — this mismatch means the project may not meet the 'Add data-qa attributes' checklist item. Either rename the selectors/attributes to data-qa or explain/justify why data-cy is used instead and update the task checklist accordingly.
| @@ -1,12 +1,48 @@ | |||
| /// <reference types='cypress' /> | |||
There was a problem hiding this comment.
The task instructions require data-qa attributes for elements used by tests, but your tests and PageObjects rely on data-cy via cy.getByDataCy. Pick one attribute name and make it consistent with the task requirements (rename attributes or update tests/helpers).
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
You're calling cy.register(...) in beforeEach which sets the authentication cookie. That means the 'Sign In' test below may start with the user already logged in—making the login flow test invalid or flaky. For tests that should exercise the login flow, create the user in the backend without setting the cookie, or explicitly log out before attempting sign-in.
| signUpPage.fillForm(user); | ||
| signUpPage.clickSignUpBtn(); | ||
|
|
||
| cy.get('.swal-title') | ||
| .should('contain', 'Welcome'); | ||
| cy.get('.swal-button').click(); | ||
|
|
||
| homePage.assertHeaderContainUsername(user.username); |
There was a problem hiding this comment.
The 'should provide an ability to log in' test attempts to log in while the beforeEach has already registered (and thus authenticated) the user. Either avoid authenticating in beforeEach for this spec or add an explicit logout step before testing the sign-in flow to ensure you're exercising the intended behavior.
| it('should not sign up with taken email', () => { | ||
| cy.register(user.email, user.username, user.password); |
There was a problem hiding this comment.
In this negative sign-up test you call cy.register(...) to create the existing user. cy.register sets the session cookie and will authenticate the test runner, which may redirect away from the sign-up page and make the test invalid. Create the preexisting user without setting the cookie (use cy.request directly or add a helper that doesn't set the cookie) so the sign-up form can be exercised properly.
| beforeEach(() => { | ||
| cy.task('db:clear'); | ||
| cy.task('generateUser').then((generatedUser) => { | ||
| user = generatedUser; | ||
| }); |
There was a problem hiding this comment.
Similar to the sign-in issue: creating a user via cy.register (which sets the cookie) in beforeEach for tests that expect to be in an unauthenticated state (or that test registration behavior) will interfere. Review tests that pre-create users and ensure they do so without authenticating when the test scenario requires an unauthenticated start state.
| return cy.get('.swal-title') | ||
| .should('contain', title); | ||
| } | ||
|
|
||
| closeModal() { | ||
| return cy.get('.swal-button').click(); | ||
| } |
There was a problem hiding this comment.
Registering the user here via cy.register(...) sets the session cookie and authenticates the test runner. Because this sign-in spec is intended to exercise the login flow, creating and authenticating the user in beforeEach will prevent the test from actually exercising sign-in. Create the user without setting the cookie (e.g. via a server-side cy.task or a cy.request that doesn't set a cookie) or clear the session cookie before attempting the login steps.
| } | ||
|
|
||
| closeModal() { |
There was a problem hiding this comment.
In beforeEach you call cy.register(...) for the test user; this sets the session cookie and authenticates the runner. For tests that expect to start unauthenticated (or that exercise authentication flows), avoid using cy.register in setup or create the user without setting the cookie.
| @@ -1,7 +1,24 @@ | |||
| class PageObject { | |||
| get usernameLink() { | |||
| return cy.getByDataCy('username-link'); | |||
There was a problem hiding this comment.
PageObject helper usernameLink uses data-cy attributes via cy.getByDataCy('username-link'). The assignment asked to add data-qa attributes for elements used in tests. Please standardize attribute naming across the app/tests (rename to data-qa or explain why data-cy is used and update the assignment checklist accordingly).
| } | ||
| } |
There was a problem hiding this comment.
The register custom command sets the session cookie (cy.setCookie('drash_sess', user.token)) which authenticates the test runner. Consider adding an alternate helper (e.g. createUser or an option on register) that creates a user in the DB without setting the cookie so tests can set authenticated/unauthenticated states intentionally.
| }).then((registeredUser) => { | ||
| user = registeredUser; | ||
| }); | ||
| cy.task('generateUser').then((generatedTargetUser) => { |
There was a problem hiding this comment.
The createArticle command's tags property is declared as string, but test helpers and generators may pass an array of tags (e.g. string[]). Consider changing the type to tags?: string | string[]; to match usage and avoid type mismatches.
|
|
||
| declare namespace Cypress { |
There was a problem hiding this comment.
The PageObject helper uses data-cy (cy.getByDataCy('username-link')) while the task description requires data-qa attributes for elements used by tests. Either rename this helper and selectors to use data-qa or document why data-cy is used and ensure the assignment's requirement is satisfied. Standardize attribute naming across components, page objects, and tests.
| getByDataCy( | ||
| selector: string | ||
| ): Chainable<any>; | ||
| register( | ||
| email: string, | ||
| username: string, | ||
| password: string | ||
| ): Chainable<any>; | ||
| login( | ||
| email: string, | ||
| password: string | ||
| ): Chainable<any>; | ||
| createArticle( | ||
| articleData: { | ||
| author_id?: number; | ||
| title: string; | ||
| description: string; | ||
| body: string; | ||
| tags?: string; | ||
| } |
There was a problem hiding this comment.
The register command posts to /users and then calls cy.setCookie('drash_sess', user.token), which authenticates the test runner as the created user. That is fine for tests that need an authenticated user, but it interferes with tests that must start unauthenticated (e.g. sign-in/sign-up negative cases). Consider adding an optional flag to cy.register to skip setting the cookie (for example cy.register(email, username, password, { setSession: false })) or provide a separate cy.task that creates users in DB without touching the browser cookie.
| description: string; | ||
| body: string; |
There was a problem hiding this comment.
The actual setCookie call here is what authenticates the browser session after registration. If you add a skip option to cy.register, gate this cy.setCookie call behind that option so tests can create users without logging in.
| getByDataCy( | ||
| selector: string | ||
| ): Chainable<any>; |
There was a problem hiding this comment.
The .d.ts declaration exposes getByDataCy(...). If you switch to data-qa as required by the task, update the helper name (and this type declaration) accordingly (for example, add getByDataQa and update signatures). Keep the typed helper in sync with whichever data-* attribute you choose.
| Cypress.Commands.add('getByDataCy', (selector) => { | ||
| return cy.get(`[data-cy="${selector}"]`); |
There was a problem hiding this comment.
The repository requirement asks to add data-qa attributes for elements used in tests, but the helper here and the rest of the tests rely on data-cy. This is a mismatch with the task description. Decide which attribute name to standardize on and make it consistent (rename the helper and selectors to data-qa or document why data-cy is used).
| password | ||
| }).then((response) => { | ||
| const user = { | ||
| ...response.body.user, | ||
| password | ||
| }; | ||
| return cy.setCookie('drash_sess', user.token) |
There was a problem hiding this comment.
Registering a user via cy.register(...) in beforeEach sets the session cookie and authenticates the browser context. Tests that are meant to exercise the sign-in page (positive and negative login cases) will start already authenticated and become invalid. Create test users without setting the cookie (e.g. via a backend cy.task that inserts directly into the DB) or clear the session cookie before the login UI interactions.
| }); | ||
|
|
||
| Cypress.Commands.add('register', (email = 'riot@qa.team', username = 'riot', password = '12345Qwert!') => { | ||
| cy.request('POST', '/users', { | ||
| email, | ||
| username, | ||
| password | ||
| Cypress.Commands.add('createArticle', (articleData) => { | ||
| return cy.getCookie('drash_sess').then((cookie) => { | ||
| const token = cookie ? cookie.value : ''; | ||
| return cy.request({ | ||
| method: 'POST', | ||
| url: '/articles', | ||
| body: { |
There was a problem hiding this comment.
The negative login test attempts to log in with wrong credentials but the beforeEach has already authenticated the session. If the app redirects away from the login page when authenticated, the test will not reach the expected error flow. Ensure the session is cleared before running this negative login attempt so the app actually tries to authenticate the provided credentials.
| }); | ||
|
|
There was a problem hiding this comment.
Pre-creating a user with cy.register(...) before opening the registration page (negative sign-up test) sets the session cookie and may redirect the client away from the registration form, making the negative registration assertion invalid. Create the preexisting user without setting a cookie (server-side insert or a request that doesn't set cookie) so the registration UI can be exercised properly.
| password | ||
| }).then((response) => { | ||
| const user = { | ||
| ...response.body.user, | ||
| password | ||
| }; | ||
| return cy.setCookie('drash_sess', user.token) | ||
| .then(() => cy.wrap(user)); | ||
| }); | ||
| } | ||
| ); | ||
|
|
||
| Cypress.Commands.add('getByDataCy', (selector) => { | ||
| cy.get(`[data-cy="${selector}"]`); | ||
| Cypress.Commands.add('login', (email, password) => { | ||
| return cy.request('POST', '/users/login', { | ||
| user: { email, password } | ||
| }).then((response) => { | ||
| const user = response.body.user; |
There was a problem hiding this comment.
Both users (the actor and the target) are created using cy.register(...) in beforeEach. Each cy.register call sets the session cookie for the last registered user; tests then call cy.login(...) to ensure the correct user is logged in, but doing DB-created users without setting cookies would be more robust and avoid any session confusion. Consider using cy.task or non-authenticating requests to create users during setup.
|
|
||
| Cypress.Commands.add('getByDataCy', (selector) => { | ||
| cy.get(`[data-cy="${selector}"]`); | ||
| Cypress.Commands.add('login', (email, password) => { | ||
| return cy.request('POST', '/users/login', { |
There was a problem hiding this comment.
In article.cy.js the cy.task('generateArticle') call is issued separately in beforeEach rather than being chained to the prior registration chain. Cypress schedules commands in order, but chaining the tasks (or returning the combined chain from beforeEach) makes the setup explicitly sequential and avoids timing ambiguity.
| return cy.get(`[data-cy="${selector}"]`); | ||
| }); |
There was a problem hiding this comment.
PageObjects and tests rely on the cy.getByDataCy helper. If you decide to comply with the task requirement to use data-qa, update this helper and all calls (and change the attributes in templates) so selectors and helper naming remain consistent with the assignment checklist.
|
|
||
| class SettingsPageObject extends PageObject { | ||
| url = '/#/settings'; | ||
|
|
There was a problem hiding this comment.
These page object getters use cy.getByDataCy('edit-article-btn'). The task requires adding data-qa attributes for elements used by tests. Please standardize on the required attribute (rename selectors/helpers to data-qa or add corresponding data-qa attributes in the templates) so the checklist item is satisfied.
| get usernameField() { | ||
| return cy.getByDataCy('username-settings'); | ||
| } | ||
|
|
There was a problem hiding this comment.
This getter uses cy.getByDataCy('delete-article-btn'). As above, convert selectors/helpers to the required attribute name (data-qa) or ensure components include data-cy consistently and document the deviation from the task requirement.
| url = '/#/settings'; | ||
|
|
||
| get usernameField() { | ||
| return cy.getByDataCy('username-settings'); |
There was a problem hiding this comment.
Title field selector uses cy.getByDataCy('title-input'). Update to use the required data-qa attribute (or add data-qa in the component) and update the helper name if you switch to getByDataQa for clarity and compliance.
| } | ||
|
|
||
| get bioField() { | ||
| return cy.getByDataCy('bio-settings'); |
There was a problem hiding this comment.
Description field uses cy.getByDataCy('description-input'). Make the attribute naming consistent with the assignment (change to data-qa or ensure components include data-cy everywhere and explain this choice).
| } | ||
|
|
||
| get emailField() { | ||
| return cy.getByDataCy('email-settings'); |
There was a problem hiding this comment.
Body field getter uses cy.getByDataCy('body-input'). Please make sure the application templates contain the matching data-* attribute you decide on (data-qa per requirements) and update PageObjects accordingly.
| } | ||
|
|
||
| get passwordField() { | ||
| return cy.getByDataCy('password-settings'); |
There was a problem hiding this comment.
Publish button getter uses cy.getByDataCy('publish-btn'). Update selectors/helpers to match the required data-qa attribute or ensure the app templates include data-cy consistently and document this deviation from the task checklist.
| get usernameField() { | ||
| return cy.getByDataCy('username-settings'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Follow button getter uses cy.getByDataCy('follow-btn'). The test-requirement asks for data-qa attributes — please standardize the attribute name across components and tests and update this getter if you switch to data-qa.
| get bioField() { | ||
| return cy.getByDataCy('bio-settings'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Unfollow button getter uses cy.getByDataCy('unfollow-btn'). Ensure the UI elements have the corresponding data-* attributes required by the assignment and update this getter if you adopt data-qa instead of data-cy.
| url = '/#/settings'; | ||
|
|
||
| get usernameField() { | ||
| return cy.getByDataCy('username-settings'); |
There was a problem hiding this comment.
Username settings field uses cy.getByDataCy('username-settings'). As with other selectors, change to data-qa or ensure the app contains data-cy everywhere and note the deviation. Also update the helper and type definitions if you rename the helper to getByDataQa.
| } | ||
|
|
||
| get bioField() { | ||
| return cy.getByDataCy('bio-settings'); |
There was a problem hiding this comment.
Bio settings field uses cy.getByDataCy('bio-settings'). Please make the test attribute naming consistent with the assignment (data-qa) and update the PageObject accordingly.
| } | ||
|
|
||
| get emailField() { | ||
| return cy.getByDataCy('email-settings'); |
There was a problem hiding this comment.
Email settings field uses cy.getByDataCy('email-settings'). Ensure the component templates include the agreed data-* attribute and update PageObjects/helpers/types if you switch to data-qa.
| } | ||
|
|
||
| get passwordField() { | ||
| return cy.getByDataCy('password-settings'); |
There was a problem hiding this comment.
Password settings field uses cy.getByDataCy('password-settings'). Standardize attribute naming across the app/tests (prefer data-qa per task) and update this getter accordingly.
| } | ||
|
|
||
| get updateSettingsBtn() { | ||
| return cy.getByDataCy('update-settings-btn'); |
There was a problem hiding this comment.
Update Settings button getter uses cy.getByDataCy('update-settings-btn'). As above, change to data-qa or ensure components include data-cy consistently. Also ensure intercept in tests matches the request path if you rename endpoints/selectors.
| } | ||
|
|
||
| get logoutBtn() { | ||
| return cy.getByDataCy('logout-btn'); |
There was a problem hiding this comment.
Logout button getter uses cy.getByDataCy('logout-btn'). Convert to data-qa or ensure data-cy exists in the templates everywhere; update the helper name (getByDataQa) and types if you change it.
|
|
||
| get titleField() { | ||
| return cy.getByDataCy('title-input'); | ||
| } | ||
|
|
||
| get descriptionField() { | ||
| return cy.getByDataCy('description-input'); | ||
| } | ||
|
|
||
| get bodyField() { | ||
| return cy.getByDataCy('body-input'); | ||
| } | ||
|
|
||
| get publishBtn() { |
There was a problem hiding this comment.
This page object uses cy.getByDataCy('edit-article-btn') and cy.getByDataCy('delete-article-btn'). The task requires adding data-qa attributes for all elements you work with in tests. Either change the selector/helper to use data-qa or add matching data-qa attributes to the components. Also the article title/body assertions use generic CSS selectors ('.article-page h1' and '.article-content .col-xs-12 div'), which are brittle — prefer adding a data-* attribute (e.g. data-qa="article-title") and selecting by that so tests don't break on layout changes.
| return cy.getByDataCy('title-input'); | ||
| } | ||
|
|
||
| get descriptionField() { | ||
| return cy.getByDataCy('description-input'); | ||
| } | ||
|
|
||
| get bodyField() { | ||
| return cy.getByDataCy('body-input'); | ||
| } | ||
|
|
||
| get publishBtn() { | ||
| return cy.getByDataCy('publish-btn'); |
There was a problem hiding this comment.
Inputs and buttons here use data-cy (title-input, description-input, body-input, publish-btn). The assignment explicitly asks for data-qa attributes; please standardize on the required attribute name. Also ensure that the application templates include the chosen data-* attributes so these getters will find the elements at runtime.
| typeTags(tags) { | ||
| cy.get('.ti-new-tag-input') | ||
| .clear(); | ||
| tags.forEach((tag) => { | ||
| cy.get('.ti-new-tag-input').type( | ||
| `${tag}{enter}` | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The tag input handling relies on the CSS class .ti-new-tag-input. If your tests interact with tags, consider adding a dedicated data-* attribute on the tag input (for example data-qa="tags-input") and use that in the page object. That will make tag interactions less dependent on third-party class names and more robust.
|
|
||
| get descriptionField() { | ||
| return cy.getByDataCy('description-input'); | ||
| } | ||
|
|
||
| get bodyField() { | ||
| return cy.getByDataCy('body-input'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Follow/unfollow buttons use data-cy selectors (follow-btn/unfollow-btn), and the bio assertion uses a CSS selector ('.user-info p'). As with other page objects, change selectors to the assignment-required data-* attribute (or update the assignment expectation). Add data-qa attributes to the follow/unfollow buttons and to the bio element so the tests target explicit attributes instead of layout-based selectors.
| return cy.getByDataCy('title-input'); | ||
| } | ||
|
|
||
| get descriptionField() { | ||
| return cy.getByDataCy('description-input'); | ||
| } | ||
|
|
||
| get bodyField() { | ||
| return cy.getByDataCy('body-input'); | ||
| } | ||
|
|
||
| get publishBtn() { | ||
| return cy.getByDataCy('publish-btn'); | ||
| } | ||
|
|
||
| typeTitle(title) { | ||
| return this.titleField.clear().type(title); | ||
| } | ||
|
|
||
| typeDescription(description) { | ||
| return this.descriptionField.clear().type(description); |
There was a problem hiding this comment.
All settings page fields and buttons are selected via data-cy (username-settings, bio-settings, email-settings, password-settings, update-settings-btn, logout-btn). The task requires data-qa attributes — please standardize and update both the page object getters and the component templates to include the appropriate data-* attributes so tests meet the checklist requirement and selectors remain stable.
|
|
||
| class ArticlePageObject extends PageObject { | ||
| get editArticleBtn() { | ||
| return cy.getByDataCy('edit-article-btn').first(); |
There was a problem hiding this comment.
This page object uses cy.getByDataCy('edit-article-btn')/delete-article-btn. The task description requires adding data-qa attributes for elements used by tests; currently your tests and page objects use data-cy. Standardize on one attribute name and update selectors/helpers or component attributes accordingly (rename to data-qa or keep data-cy but note this difference in the PR).
| get editArticleBtn() { | ||
| return cy.getByDataCy('edit-article-btn').first(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The editor page object selects fields via cy.getByDataCy('title-input'), description-input, body-input, and publish-btn. As above, these rely on data-cy attributes; to meet the assignment's explicit requirement, either change these selectors to target data-qa attributes or add data-qa attributes to the corresponding templates and update the helper.
| } | ||
|
|
||
| get deleteArticleBtn() { | ||
| return cy.getByDataCy('delete-article-btn').first(); |
There was a problem hiding this comment.
Profile page object uses cy.getByDataCy('follow-btn') and unfollow-btn. Ensure the UI components include the matching data-* attributes and consider aligning them with the data-qa naming required by the task (or update the task checklist if you intentionally use data-cy).
| get editArticleBtn() { | ||
| return cy.getByDataCy('edit-article-btn').first(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Settings page object accesses multiple controls via cy.getByDataCy (username-settings, bio-settings, email-settings, password-settings, update-settings-btn, logout-btn). These are fine, but they must match the attribute naming convention mandated by the assignment (data-qa). Please make selectors and component attributes consistent across the codebase.
|
|
||
| class ProfilePageObject extends PageObject { | ||
| visit(username) { | ||
| cy.visit(`/#/@${username}`); |
There was a problem hiding this comment.
This Page Object uses data-cy('edit-article-btn'). The task requirements require adding data-qa attributes for elements used in tests. Please standardize on one attribute name across components and tests (rename either the attributes in templates to data-cy → data-qa, or update tests/helpers to use data-qa) so the project meets the assignment checklist.
| visit(username) { | ||
| cy.visit(`/#/@${username}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
This file selects form fields by data-cy (e.g. title-input). The assignment asks for data-qa attributes — make the attribute name consistent across app templates, page objects and the getByDataCy helper (or rename the helper if you switch to data-qa).
| } | ||
|
|
||
| get followBtn() { | ||
| return cy.getByDataCy('follow-btn'); |
There was a problem hiding this comment.
Profile page selectors rely on data-cy (follow-btn, unfollow-btn). Ensure the components include matching attributes and consider switching to the required data-qa naming if you want to satisfy the task checklist.
| visit(username) { | ||
| cy.visit(`/#/@${username}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
Settings PageObject uses data-cy selectors (e.g. username-settings). Confirm all corresponding component elements have these attributes, and consider renaming to data-qa (or updating the assignment justification) so the project explicitly meets the "Add data-qa attributes" requirement.
|
|
||
| typePassword(password) { |
There was a problem hiding this comment.
Postgres is published to the host on port 54320:5432. If you don't need host access to Postgres (for example, in CI or when services talk to each other inside the compose network) you can remove this host port mapping to reduce exposure and avoid confusion. Keep this mapping only when you explicitly require connecting from the host.
|
|
||
| typePassword(password) { |
There was a problem hiding this comment.
The Postgres service is published to the host on port 54320:5432 in this file as well. Consider removing or restricting this mapping if host access is not required, to avoid accidental host-dependent behavior.
|
|
||
| apache: |
There was a problem hiding this comment.
PageObject uses cy.getByDataCy('username-link') while the task requires adding data-qa attributes for elements used in tests. This is a mismatch with the assignment checklist. Either change the helper and selectors to target data-qa or add data-cy attributes across components and explicitly document the deviation. Prefer using the required data-qa so the submission meets the checklist unambiguously.
| - POSTGRES_PASSWORD=userpassword | ||
| healthcheck: | ||
| test: ["CMD-SHELL", "pg_isready -U user"] |
There was a problem hiding this comment.
In cypress.config.js the code calls faker.lorem.words({ min: 2, max: 5 }). Many versions of @faker-js/faker expect a numeric count for lorem.words, not an options object; passing an object may throw. Generate a random integer for the words count (for example const count = faker.number.int({ min: 2, max: 5 })) and then call faker.lorem.words(count).split(' ') to produce the tags array.
| this.typeEmail(email); | ||
| this.typePassword(password); | ||
| } | ||
|
|
There was a problem hiding this comment.
This volume line - /var/www/src/node_modules is ambiguous and often incorrect: it will be interpreted as a host absolute path, not an anonymous volume or a container path mapping. If the intent is to keep container node_modules isolated, use a named volume such as - node_modules:/var/www/src/node_modules and declare the named volume under volumes:. If you intended to bind the host's node_modules, use an explicit host path like ./node_modules:/var/www/src/node_modules instead.
|
|
||
| typeUsername(username) { | ||
| return this.usernameField.clear().type(username); | ||
| } |
There was a problem hiding this comment.
You define a Postgres healthcheck using pg_isready -U user. This is helpful to gate dependent services. Just verify the chosen Postgres image contains pg_isready (official images do) and that the -U user matches the POSTGRES_USER environment variable so the healthcheck is reliable.
| this.typeEmail(email); | ||
| this.typePassword(password); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same issue as above: - /var/www/src/node_modules appears here as well. Please convert to a named or explicit host-mounted volume to avoid accidentally mounting an unexpected host path into the container.
|
|
||
| services: | ||
|
|
||
| apache: |
There was a problem hiding this comment.
The PageObject helper selects the header username using cy.getByDataCy('username-link'). The task description explicitly requires data-qa attributes for elements used in tests. Standardize the data-* attribute (choose data-qa per the assignment or update the assignment justification) and update this helper and all PageObjects accordingly.
| command: bash -c "npm i && npm run webpack & deno run --allow-net --allow-read --unstable app.ts" | ||
| depends_on: | ||
| postgres: | ||
| condition: service_healthy |
There was a problem hiding this comment.
When creating articles via the helper you pass tags: articleData.tags || ''. Tests/generators sometimes produce arrays of tags. Normalize the tags value to the format the backend expects (string or comma-separated) here and consider accepting string | string[] in the helper to be robust.
| environment: | ||
| - POSTGRES_USER=user | ||
| - POSTGRES_PASSWORD=userpassword | ||
| healthcheck: | ||
| test: ["CMD-SHELL", "pg_isready -U user"] |
There was a problem hiding this comment.
You also register targetUser via cy.register(...) in the same beforeEach. Each cy.register call sets the session cookie, so after creating both users the browser cookie will belong to the last registered user. This can produce confusing state. Prefer creating users without changing the session cookie during setup, then explicitly log in with cy.login(...) in each test as needed.
| - POSTGRES_PASSWORD=userpassword | ||
| healthcheck: | ||
| test: ["CMD-SHELL", "pg_isready -U user"] |
There was a problem hiding this comment.
In cypress.config.js you call faker.lorem.words({ min: 2, max: 5 }). Many versions of @faker-js/faker expect a numeric count argument rather than an options object. Use a numeric count (for example const count = faker.number.int({ min: 2, max: 5 }) and faker.lorem.words(count)) so tags becomes a stable string to split into an array.
| <!-- Used when user is also author --> | ||
| <span v-if="canModify"> | ||
| <router-link class="btn btn-sm btn-outline-secondary" :to="editArticleLink"> | ||
| <router-link class="btn btn-sm btn-outline-secondary" :to="editArticleLink" data-cy="edit-article-btn"> |
There was a problem hiding this comment.
This router-link includes a data-cy attribute. The task requires data-qa attributes for elements used in tests — please standardize the attribute name across the app and tests (rename attributes or update tests/helpers) so the checklist item is satisfied.
| host: 'localhost', | ||
| dialect: 'postgres', | ||
| port: 54320, |
There was a problem hiding this comment.
Hard-coded DB host/port (host: 'localhost', port: 54320) will not work when the app runs inside Docker Compose (the DB is reachable by the postgres service name within the network). Make the DB host/port configurable via environment variables and use the compose service hostname (or env var) so the app can connect both locally and inside containers.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
The test task has already been approved.
[CODE: AAP]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
2 participants
Description