Feature: e2e tests

[UlisesGascon]

Ready for review (See #142 )

[ckarande]

@UlisesGascon Awesome! Great work with e2e setup and login page tests. This has been in my wishlist for a long time and great to see the progress you have made towards it 👍.

Yes, good idea to have a dev branch. We can expand the contributor's guide section on the README.md explaining the proposed git workflow to follow for PRs.

[binarymist]

In regards to test location, would it make more sense to put these tests in /test/endTwoEnd/ (or what ever the dir naming convention is), or /test/integration/, or even /test/integration/cypress/ at worst case?

In terms of coding to interface rather than implementation and the Liskov substitution principle I'm not sure the dir should be named after the technology as opposed to the intent of the tests?

[ckarande]

@UlisesGascon @KoolTheba ..Great Work..🎉🎉
Thank you so much for this valuable addition to Nodegoat.

I like the way you organized the tests and covered the key features. I have just a few of minor thoughts:

• On running the package.json script test:e2e from terminal didn't start the execution of the NodeGoat tests for me. I think the test runner looks for the cypress folder with tests at the project root level. I got the test running by moving them to root and fixing the env path but I am sure this is not the intended way. Can you please add a section in the README.md with specific instructions you follow to allow the test runner to locate the tests in tests/e2e folder when running npm run test:e2e command?
• Regarding the failing tests for the/profile endpoint, the profile feature is not intended for an admin user. On the UI, there is not direct link for an admin user to access the profile page. It is true that an admin user can explicitly open the profile url in the browser address bar (as the tests are doing) and access the page that way, but that is not the path we need to add tests for. In fact, besides the benefits, login, and logout features, no other features are intended for the admin user. So we can remove the test Should be accesible if the user is an admin from all other behaviors including profile, learn, contributions, memos, research, allocations, and dashboard.
• Do you find the Triple slash directives or tsconfig type declaration helpful to get the auto complete hints in VS Code while writing the tests? If so, can we add it?

Rest of the tests and setup looks great to me 👍

[UlisesGascon]

Thanks for the feedback @ckarande 👍

• @KoolTheba will refactor the test
• I love your ideas for VSCode.
• I will check the bug in the test:e2e part.

[ckarande]

Thanks @UlisesGascon & @KoolTheba.

[UlisesGascon]

@ckarande please check if the tests are working as expected, I forgot to add the config file in the previous commits 😓

[KoolTheba]

@ckarande I think all the changes you suggested are done 💪 Please review! ^^

[ckarande]

@UlisesGascon @KoolTheba, fantastic! Thank you very much for your efforts on suggested changes.

I had two failing tests for the profile page. The Should first name be modified test is failing correctly because changes to the first name field not saving correctly. This is a bug in the application itself (great catch by tests. I didn't even notice this defect earlier) and I have opened an issue #145 for it.
I will take care of it at some point, but for the failing test, can we update it to use the last name or any other field on the profile page, so that the issue #145 won't block starting on the travis CI integration feature(#144).

The second test failure came from the Google search this profile by name test. This test seems to look for a keyword 'google' in the href attribute of the anchor tag. Should it look for it in the body of the anchor tag instead?

Other than above two items, rest of the changes look great to me. Well done. I am very excited and looking forward to include this test suite as part of the NodeGoat codebase! 🥇

[UlisesGascon]

Thanks for the feedback @ckarande! :-)

I will skip Should first name be modified inside the PR for #145. I like as well that we discovered already a bug just testing ^^.

Regarding Google search this profile by name I am not so sure what is the expected behaviour. The current generated link is not a valid link. I mean… the idea is generate a link for google yourself like: https://www.google.es/search?q=node+goat?

[ckarande]

@UlisesGascon I believe the idea behind this link was to just include a malicious payload (user supplied firstname in this case) to demo XSS in the URL context. It doesn't really go to google for search. @lirantal please suggest if you recall this. At some point we should include more details on this in the tutorial.

[lirantal]

Yep as @ckarande pointed out, the idea was to show an XSS in a URL context (so to apply proper encoding). We didn't really care that it's not a valid google search link.

[UlisesGascon]

Thanks for the explanation @ckarande @lirantal. I will remove that check from the expect. I believe that we can merge the PR after that ;-)

[UlisesGascon]

Ready! 🎉

[ckarande]

@UlisesGascon Looks great. I see the first name related test passing, but I have one test failure for Google search this profile by name.

Apparently the test is looking for "target" attribute present in the anchor tag, but it is not in the template.

<a href="{{firstNameSafeString}}">Google search this profile by name</a>

Should we just remove this assert?.should('have.attr', 'target', '_blank')

[UlisesGascon]

All ready @ckarande. I removed the target validation part :-)

[ckarande]

Awesome @UlisesGascon . I am merging the PR. Thank you for your valuable contribution 🎉

[UlisesGascon]

Thanks for the merge! I will keep pushing changes to the repo after this weekend. I was focus on personal matters the last few weeks, sorry for the delay ;-)

[ckarande]

No worries at all. In fact, thanks for managing time for it.👍
As a next step, should we focus on the Travis CI integration to run these tests?

[UlisesGascon]

Yeah! I am very close to send the PR, I just detected an issue with Travis and Cypress regarding child processes. But I hope that very soon I will be able to send the PR :-)