-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[MM-27209] - Add spec for archived channels part 03 #7209
Conversation
cy.apiAdminLogin(); | ||
cy.apiUpdateConfig({ | ||
ServiceSettings: { | ||
ExperimentalChannelSidebarOrganization: 'default_off', |
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.
In hindsight, the more I see us doing this the more I wonder if we should just change the test procedure to use the new sidebar instead. What do you think/how hard is that to do?
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.
The ticket has tests for both new and old sidebar, and for example wants to test old sidebar specific features like channel grouping 🤷♀️ If we are getting rid of the old sidebar soon then these test cases will be redundant then too but for now I guess we are keeping them?
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.
That's true, though I'm wondering what the value is in writing these tests for the old sidebar. Especially considering that the main purpose for the push in writing these tests is for cloud, which will never be using the old sidebar. Let's chat about this in triage.
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.
Agreed.
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.
@nevyangelova Could you please run the tests and confirm if all are passing on you. Most are failing on me.
Also after system config change, I suggests to use a regular user for testing (instead of sysadmin) as much as possible so we can ensure that it starts with the same expected user setting, and will not be affected by other test's change.
@saturninoabril @devinbinnie updated all tests to use the new sidebar. Removed Admin login and left it only for creating private channels. All tests are passing for me. Updated the test cases :) Removed MM-T1698 as an old sidebar feature test only. |
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 @nevyangelova! Please see comments.
// # More channels list should contain the archived channel | ||
cy.get('#moreChannelsList').should('contain', archivedChannel.display_name); | ||
}); | ||
}); |
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 getting failing tests here, probably due to sysadmin's user preference. If possible, after config or other requests, please use regular user instead of sysadmin. Also, does the config change at MM-T1696 applicable to the succeeding tests. If yes, then I suggest to move all new tests to new spec file and have the config change in the before
hook.
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.
Also, does the config change at MM-T1696 applicable to the succeeding tests. If yes, then I suggest to move all new tests to new spec file and have the config change in the before hook.
No, we only need it for that one test case to confirm that when archived channels feature is disabled users cannot viewthem.
@saturninoabril fixed, removed all admin user apart from MM-T1696 since that needs to change the config for the test, and I moved it below the other test cases because of the different config settings. |
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 Nevy! LGTM, tested and all passed.
/update-branch |
Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour. |
Trying to auto merge this PR. |
Pull Request successfully merged |
Summary
This PR adds 7 cases (for web) from the archived channels part 03 epic
Ticket Link
https://mattermost.atlassian.net/browse/MM-27209