Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

[MM-14242] Remove props such as lastPostCount, idCount and idPrefix which were previously used for Se E2E #2410

Merged
merged 2 commits into from
Feb 28, 2019

Conversation

saturninoabril
Copy link
Member

Summary

Due to some limitation of Se to identify post ID, dynamic IDs for Post component were introduced to make the last post/s be testable. Props were added for that purpose such as lastPostCount, idCount and idPrefix. However, those dynamic IDs were eventually disabled due to unnecessary re-rendering of components on change of props (counts changed whenever new post/s arrived/submitted). Related Se tests were eventually either disabled or put on manual tests.

With Cypress, we have now the ability to get post ID like cy.getLastPostId() or in the future we can add command like cy.getPostId(xNumberFromRecent).

This PR removed props such as lastPostCount, idCount and idPrefix. This also aimed to organize how to assign ID into a component. Naming convention is like [location]_[name]_[post_id] where location is either on CENTER, RHS_ROOT, RHS_COMMENT or SEARCH.

Ticket Link

Jira ticket: MM-14242

Checklist

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Added or updated unit tests (required for all new features)
  • Updated E2E

@saturninoabril saturninoabril added the 2: Dev Review Requires review by a core commiter label Feb 26, 2019
@saturninoabril saturninoabril added this to the v5.10.0 milestone Feb 26, 2019
@saturninoabril saturninoabril requested review from migbot, sudheerDev and jespino and removed request for sudheerDev February 26, 2019 11:11
tests/setup.js Outdated
@@ -64,7 +64,7 @@ beforeEach(() => {

afterEach(() => {
if (logs.length > 0 || warns.length > 0 || errors.length > 0) {
throw new Error('Unexpected console logs' + logs + warns + errors);
// throw new Error('Unexpected console logs' + logs + warns + errors);
Copy link
Member

Choose a reason for hiding this comment

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

Why are you disabling it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, sorry, nice catch. That's for my own debugging only. Will revert that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @jespino, now updated.

Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Everything looks great, except the disabling of the checking for output in the tests to fail.

Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

LGTM

@saturninoabril saturninoabril merged commit 4dce36d into mattermost:master Feb 28, 2019
@saturninoabril saturninoabril deleted the MM-14242 branch February 28, 2019 19:39
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Mar 19, 2019
@lindy65 lindy65 removed the 2: Dev Review Requires review by a core commiter label Mar 29, 2019
@DHaussermann DHaussermann added the Tests/Not Needed Does not require new release tests label Mar 29, 2019
stevepartridge pushed a commit to stevepartridge/mattermost-webapp that referenced this pull request Mar 30, 2019
…hich were previously used for Se E2E (mattermost#2410)

* Remove props such as lastPostCount, idCount and idPrefix which were previously used for Se E2E

* revert commented out test setup
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed Does not require new release tests
Projects
None yet
6 participants