Skip to content

Add user logout routes to test files - #1691

Open
Nespina24 wants to merge 10 commits into
OpenEnergyDashboard:developmentfrom
Nespina24:issue-1683-User-Logouts
Open

Add user logout routes to test files#1691
Nespina24 wants to merge 10 commits into
OpenEnergyDashboard:developmentfrom
Nespina24:issue-1683-User-Logouts

Conversation

@Nespina24

@Nespina24 Nespina24 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

This PR cleans up tests suites by adding in a logout route to test users who login. When a test logs in, it should also clean up by logging out, so the authentication lifecycle follows a consistent pattern (login → perform the test → logout). This PR addresses the issue in the following files:

src/server/test/routes/logsRouteTests.js
src/server/test/routes/unitsRouteTests.js
src/server/test/web/login.js
src/server/test/web/usersTest.js
src/server/test/web/groups.js
src/server/test/web/maps.js
src/server/test/web/meters.js
src/server/test/web/preferencesTest.js

Fixes #1683

Type of change

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

No limitations.

@Nespina24
Nespina24 marked this pull request as draft August 4, 2026 17:58
@Nespina24

Copy link
Copy Markdown
Contributor Author

I drafted this PR for now since I am not currently finished implementing all the logouts, but will undraft it once I am finished.

I just had a few clarifying questions about some of the tests.

Comment thread src/server/test/web/meters.js Outdated
mocha.before(async () => {
// login
let res = await chai.request(app).post('/api/loginLogout/login')
.send({ username: testUser.username, password: testUser.password });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If this is supposed to test both ADMIN and CSV roles, then it looks like it doesn't access the CSV role. The test looks like it simply runs ADMIN both times instead of depending on the role being looped. If this behavior is unintended, then I can make the appropriate changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What I see seems very similar to the next test for the other roles:

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) {

I see what you mean and you seem correct. I think it should login with the desired role and use that token. Thanks for offering to make that change.

Unlike the next test which should test everyone else, this one should only test these roles. Thus, I think it would be much better to check if the role is admin or csv rather than not the others. Then it will still work if more roles are added. If you agree, could you also do that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I adjusted the test to login with the same role, but I noticed that it would only pass if I changed the before hook to a beforeEach hook in src/server/test/web/meters.js on line 162. Otherwise, I kept getting the following errors for both admin and csv:

 21 passing (35s)
  2 failing

  1) meters API
       Admin role & CSV role:
         should return all meters for ADMIN:
     AssertionError: expected { id: 1, name: null, url: null, …(31) } to have property 'name' of 'Meter 1', but got null
      at expectMetersToBeEquivalent (src/server/test/web/meters.js:54:26)
      at Context.<anonymous> (src/server/test/web/meters.js:213:6)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

  2) meters API
       Admin role & CSV role:
         should return all meters for CSV:
     AssertionError: expected { id: 1, name: null, url: null, …(31) } to have property 'name' of 'Meter 1', but got null
      at expectMetersToBeEquivalent (src/server/test/web/meters.js:54:26)
      at Context.<anonymous> (src/server/test/web/meters.js:213:6)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

I tested and found that the same error message is given for export and obvius roles for the same test, regardless of whether before or beforeEach is used (This might be the expected outcome, but shouldn't be for admin or csv). I also found that there's no difference between using an after or afterEach hook on line 176, but I kept it as afterEach to be consistent. I'm unsure of why I would strictly need a beforeEach for this test, but I plan on leaving it like this unless you have any input.

Comment thread src/server/test/web/maps.js Outdated
@Nespina24

Nespina24 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

At the time I'm writing this, the build checks are still running well past what it normally does. It gave the error "Error: write EPIPE" which I never saw while running on my local machine. Can you force stop it?

Edit: I just decided to close and reopen for now. Hopefully it won't give the same error.

@Nespina24 Nespina24 closed this Aug 5, 2026
@Nespina24 Nespina24 reopened this Aug 5, 2026
@Nespina24
Nespina24 marked this pull request as ready for review August 6, 2026 17:59
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.

Add user logouts to authentication tests

2 participants