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

Mock Oauth Unit Tests #158

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Mock Oauth Unit Tests #158

wants to merge 6 commits into from

Conversation

tkomarlu
Copy link
Contributor

@tkomarlu tkomarlu commented Jun 5, 2020

This pr adds unit tests that mock the user authentication services and verify that users can access the correct endpoints.

@@ -1,5 +1,5 @@
spring.jpa.show-sql=true
spring.jpa.hibernate.ddl-auto=update
app.admin.emails=phtcon@ucsb.edu
app.admin.emails=tkomarlu@ucsb.edu
Copy link
Collaborator

Choose a reason for hiding this comment

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

I could be mistaken, but I think this supports a comma separated list. So I wonder if you might consider:

app.admin.emails=phtcon@ucsb.edu,tkomarlu@ucsb.edu

Also, you can can override this at run time, which might be a better way of handling this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding a line to the README.md that explains that you can override app.admin.emails in localhost.json

// Now check that the name in the header is Joe
mvc.perform(MockMvcRequestBuilders.get("/").accept(MediaType.TEXT_HTML)).andExpect(status().isOk())
.andExpect(content().contentType("text/html;charset=UTF-8"))
.andExpect(xpath("/html/body/div[@class='container']/nav[@class='navbar navbar-expand-lg navbar-light bg-light']/div[@id='navbarTogglerDemo03']/ul[@class='nav navbar-nav navbar-right']/li[1]/a")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to try the least specific selector that will find the content. Otherwise, the tests become brittle ... i.e. any tiny change to the HTML causes the test to break. At some, you are just writing the code twice...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely see if you can find the least specific selector that gets the job done.

Copy link
Collaborator

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

Consider the feedback, otherwise LGTU

@@ -1,5 +1,5 @@
spring.jpa.show-sql=true
spring.jpa.hibernate.ddl-auto=update
app.admin.emails=phtcon@ucsb.edu
app.admin.emails=tkomarlu@ucsb.edu
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider adding a line to the README.md that explains that you can override app.admin.emails in localhost.json

// Now check that the name in the header is Joe
mvc.perform(MockMvcRequestBuilders.get("/").accept(MediaType.TEXT_HTML)).andExpect(status().isOk())
.andExpect(content().contentType("text/html;charset=UTF-8"))
.andExpect(xpath("/html/body/div[@class='container']/nav[@class='navbar navbar-expand-lg navbar-light bg-light']/div[@id='navbarTogglerDemo03']/ul[@class='nav navbar-nav navbar-right']/li[1]/a")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely see if you can find the least specific selector that gets the job done.

@tkomarlu tkomarlu requested a review from pconrad June 5, 2020 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants