Repository navigation
Add user logout routes to test files #1691
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
Open
Nespina24
wants to merge
20
commits into
OpenEnergyDashboard:development
Choose a base branch
from
Nespina24:issue-1683-User-Logouts
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 10 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
4cc953f
add logout to unitsRouteTests.js
Nespina24 13206a2
add logout to usersTest.js and fix some formatting
Nespina24 badd17e
add logout to maps.js and revise some comments
Nespina24 5c3b0a5
create describe block for admin and add logout to preferencesTest.js
Nespina24 7b6d28a
add logout to meters.js and remove some blank lines
Nespina24 fa38778
change meters.js to use desired role, delete some comments
Nespina24 9a67537
Add logout after each test to logsRouteTests.js
Nespina24 db34c64
temporary fix to login/logout behavior
Nespina24 714b4fb
add logout to groups.js
Nespina24 6de5ae4
add logout to login.js tests
Nespina24 59fd32c
Merge branch 'OpenEnergyDashboard:development' into issue-1683-User-L…
Nespina24 9af5064
Merge branch 'OpenEnergyDashboard:development' into issue-1683-User-L…
Nespina24 a5fe6fd
Merge branch 'OpenEnergyDashboard:development' into issue-1683-User-L…
Nespina24 46c4796
Replace before/after hooks to beforeEach/afterEach hooks
Nespina24 191bab3
Separate admin and non-admin roles in create/edit endpoints
Nespina24 be817b6
Merge branch 'OpenEnergyDashboard:development' into issue-1683-User-L…
Nespina24 633824a
Merge branch 'OpenEnergyDashboard:development' into issue-1683-User-L…
Nespina24 c9a7715
fix indentation, const declarations, password strings
Nespina24 2e90d35
Merge branch 'OpenEnergyDashboard:development' into issue-1683-User-L…
Nespina24 d522f51
Merge branch 'OpenEnergyDashboard:development' into issue-1683-User-L…
Nespina24 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -157,16 +157,31 @@ mocha.describe('meters API', () => { | |
|
|
||
| mocha.describe('Admin role & CSV role:', () => { | ||
| for (const role in User.role) { | ||
| if (User.role[role] !== User.role.OBVIUS && User.role[role] !== User.role.EXPORT) { | ||
| if (User.role[role] == User.role.ADMIN || User.role[role] == User.role.CSV) { | ||
| let token; | ||
| // Since this .before is in the middle of tests, it should not have issues as | ||
| // documented in usersTest.js. | ||
| mocha.before(async () => { | ||
| let res = await chai.request(app).post('/api/loginLogout/login') | ||
| .send({ username: testUser.username, password: testUser.password }); | ||
| token = res.body.token; | ||
| }); | ||
| mocha.it('returns all meters', async () => { | ||
| mocha.beforeEach(async () => { | ||
| // insert test user | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. space indenting should be tabs. |
||
| const conn = testDB.getConnection(); | ||
| const password = 'password'; | ||
| const hashedPassword = await bcrypt.hash(password, 10); | ||
| const authorizedUser = new User(undefined, `${role}@example.com`, hashedPassword, User.role[role]); | ||
| await authorizedUser.insert(conn); | ||
| authorizedUser.password = password; | ||
|
|
||
| // login | ||
| let res = await chai.request(app).post('/api/loginLogout/login') | ||
| .send({ username: authorizedUser.username, password: authorizedUser.password }); | ||
| token = res.body.token; | ||
| }); | ||
| mocha.afterEach(async () => { | ||
| // logout | ||
| if (token) { | ||
| await chai.request(app).post('/api/loginLogout/logout') | ||
| .set('token', token); | ||
| } | ||
| }); | ||
|
|
||
| mocha.it(`should return all meters for ${role}`, async () => { | ||
| const conn = testDB.getConnection(); | ||
| await new Meter(undefined, 'Meter 1', '1.1.1.1', true, true, Meter.type.MAMAC, '+01', gps, | ||
| 'Identified 1', 'notes 1', 10.0, true, true, '01:01:25', '05:05:05', 5.1, 7.3, 1, 'increasing', false, | ||
|
|
@@ -217,7 +232,14 @@ mocha.describe('meters API', () => { | |
| .send({ username: unauthorizedUser.username, password: unauthorizedUser.password }); | ||
| token = res.body.token; | ||
| }); | ||
|
|
||
| mocha.afterEach(async () => { | ||
| // logout | ||
| if (token) { | ||
| await chai.request(app).post('/api/loginLogout/logout') | ||
| .set('token', token); | ||
| } | ||
| }); | ||
|
|
||
| mocha.it('should only return visible data', async () => { | ||
| const conn = testDB.getConnection(); | ||
| await new Meter(undefined, 'Meter 1', '1.1.1.1', true, true, Meter.type.MAMAC, '+01', gps, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this test, I wanted to apply the potential change I noted in the issue description:
Even though it doesn't have multiple users, I thought this would be a good place to login/logout for every test since it runs for a long time. However, when I tried changing to beforeEach/afterEach, the checks had continually failed.
I'm ok with leaving it like this, but I think this should be addressed if the root cause of the failures can be found.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually considering removing the changes to this file entirely. After implementing this, I've noticed that even though it passed the most recent test, the node checks have a chance of failing different tests for a reason I'm unaware of. If it gets merged, I'm afraid that other people will start seeing similar issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious. Do you want to wait until I review the PR to see if I have any ideas? One test should not, in a perfect world, impact another test. It makes me wonder if something else is wrong that really should be tracked down. If it cannot be tracked down during this work then I suspect a new issue should be opened to look at this further. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with opening a new issue if it doesn't get resolved after a code review. If that's the case, then I think I should comment out the changes with a TODO describing the problem.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the tests in this file to use beforeEach/afterEach. I ran the single file of tests and the complete suite of tests multiple times but have yet to see it fail. Thus, I wanted to ask if you are still seeing this. If so, how often does it happen and is there anything special in your setup?
260921 update: This may be related to another comment on before/afterEach. I'm not adding any info but leaving it as part of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for responding. I also don't see it on my local machine, but I was referring to the GitHub testing suite which seems to either successfully pass or give an error on different files, such as
src/server/test/web/csvPipelineTest.jsorsrc/server/test/routes/logsRouteTests.jseven though it ran mostly the same code.Recently it seems to be working on GitHub, but it could potentially be an issue if other people pull in these changes and it returns a phantom failure.
I'll change the code to beforeEach/afterEach for now and see if the problem persists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made another comment about a potential issues I could not resolve. If this is not figured out soon then I propose to create an issue that documents the code you add (probably via a commit id) and discussed what is happening. Then it can be addressed later since there is a good chance it is not a direct result of this work. How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. I think I have a better understanding of what's happening now, but I agree that this could be a separate issue. I'm fine with creating an issue referencing the commit and including the debug output and any relevant information about the Mocha hook behavior.