Skip to content

Signup flow#246

Closed
shreya-mishra wants to merge 12 commits into
developfrom
signupFlow
Closed

Signup flow#246
shreya-mishra wants to merge 12 commits into
developfrom
signupFlow

Conversation

@shreya-mishra
Copy link
Copy Markdown
Contributor

image

Copy link
Copy Markdown
Contributor

@MehulKChaudhari MehulKChaudhari left a comment

Choose a reason for hiding this comment

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

Congrats on learning testing. I have added comments please a have look.

Comment thread app/templates/signup.hbs
@@ -47,6 +47,7 @@
{{/each}}
{{#if this.isSubmitDisabled }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can achieve both the buttons with the if condition on the disabled attribute

Comment thread tests/acceptance/signup-flow-test.js Outdated
Comment on lines +36 to +37
assert.dom('[data-test-id="form-input-first_name"]').hasProperty('value','test')
assert.dom('[data-test-id="form-input-last_name"]').hasProperty('value','user')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hasProperty checks the properties of the selected HTML element, hasProperty is used to check let's say whether the input field is a type password or a normal input field etc.

To check the value of input hasValue is the assertion to use.

Comment thread tests/acceptance/signup-flow-test.js Outdated

setupApplicationTest(hooks);

test('http://localhost:4200/signup', async function(assert) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think a more descriptive name test name will be good instead of http://localhost:4200/signup

Comment thread tests/acceptance/signup-flow-test.js Outdated
setupApplicationTest(hooks);

test('http://localhost:4200/signup', async function(assert) {
const fields = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: We can move fields to mocks folder.

Comment thread tests/acceptance/signup-flow-test.js Outdated
assert.dom('[data-test-id="signup-button"]').exists()

await this.pauseTest()
window.alert = originalWindowAlert;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why does the test have an alert? 🤔

What does this do for our assertions?

<div class="field">
<label for={{@for}}>{{@label}}:</label>
<Input
data-test-id="form-input-{{@id}}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be a static named id, so that we can easily grab it in the tests.

Suggested change
data-test-id="form-input-{{@id}}"
data-test-id="form-input"

Comment thread app/templates/signup.hbs
Copy link
Copy Markdown
Contributor

@ivinayakg ivinayakg left a comment

Choose a reason for hiding this comment

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

On starting with testing 🤩
"Welcome to the mafia"

Comment thread tests/acceptance/signup-flow-test.js Outdated
}

const originalWindowAlert = window.alert;
window.alert = function() { return true;}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need this?
(im not sure 100% honestly)


assert.dom('[data-test-id="signup-button-disabled"]').doesNotExist()
assert.dom('[data-test-id="signup-button"]').exists()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can there also be something like clicking the button and then disabling it, and checking for the same?

@rohan09-raj
Copy link
Copy Markdown
Contributor

New PR has been raised for the same issue #269

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants