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

Signup flow #246

Closed
wants to merge 12 commits into from
Closed

Signup flow #246

wants to merge 12 commits into from

Conversation

shreya-mishra
Copy link
Contributor

image

Copy link
Member

@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.

@@ -47,6 +47,7 @@
{{/each}}
{{#if this.isSubmitDisabled }}
Copy link
Member

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 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
Member

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.


setupApplicationTest(hooks);

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

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

setupApplicationTest(hooks);

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

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.

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

await this.pauseTest()
window.alert = originalWindowAlert;
Copy link
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?

@@ -1,6 +1,7 @@
<div class="field">
<label for={{@for}}>{{@label}}:</label>
<Input
data-test-id="form-input-{{@id}}"
Copy link
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"

app/templates/signup.hbs Show resolved Hide resolved
Copy link
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"

}

const originalWindowAlert = window.alert;
window.alert = function() { return true;}
Copy link
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
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
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.

5 participants