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

[4] Version control change after release note deletion #13

Merged
merged 6 commits into from
Jan 10, 2024

Conversation

johnmunster
Copy link
Contributor

@johnmunster johnmunster commented Dec 11, 2023

Description

Test for #4 - Created a test that verifies the version number in the admin bar changes accordingly after the current release note has been deleted from the post list screen.

Change Log

  • Added a test to delete release note from the release notes post list screen and verify the version number change in the admin bar.
  • Changed test:e2e script to include a complete stop of the container and to open Cypress without additional commands needing to be run in the terminal
  • Added the ReleaseNoteSeeder file

Steps to test

Checkout to the branch
run npm run test:e2e to start up the docker container and open Cypress
Choose End to End testing
Run release notes version control test

Screenshots/Videos

Successful run of the test - http://bigbite.im/v/wYjf6I

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@johnmunster johnmunster requested a review from g-elwell December 11, 2023 16:52
@johnmunster johnmunster self-assigned this Dec 11, 2023
John Munster and others added 2 commits December 12, 2023 14:43
…hanged the name of the file to be more specific to the current release note as the new assertion should only occur when the most recent release note is delete
Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

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

I'm unsure how much value this test adds for us, as it's testing core WP behaviour of deleting a post. We might want to re-think what we're trying to do here.

Based on the assertion against the toolbar version number, perhaps a test to ensure this reflects the most recent release note version number would be more suitable?

@johnmunster
Copy link
Contributor Author

I thought the same to fair, but I was trying to think of tests to write and its always good to cover CRUD I suppose. The assertion is suppose to be checking against the most recent release so I thought it was OK, but I'll check it again.

@g-elwell
Copy link
Member

g-elwell commented Jan 2, 2024

I thought the same to fair, but I was trying to think of tests to write and its always good to cover CRUD I suppose. The assertion is suppose to be checking against the most recent release so I thought it was OK, but I'll check it again.

Fair point, I wouldn't be dead set against including this if you want to continue with it

@johnmunster
Copy link
Contributor Author

I'd like to keep it, but it does need some refactoring. I think the assertion isn't great, as I'd like it assert that the version number has changed to the previous release note but at the moment there is no way to check that from the post list screen, so will need revisiting in the future.

@g-elwell
Copy link
Member

g-elwell commented Jan 5, 2024

No worries. If you're planning to refactor please convert the PR to draft and we can re-visit reviewing it when ready.

@johnmunster
Copy link
Contributor Author

Can I get it merged into main before I refactor it, as I have a feeling some other tests on the main branch might impact how I refactor it.

@johnmunster
Copy link
Contributor Author

I've changed the test as per our conversation yesterday and resolved the conflict

@johnmunster johnmunster requested a review from g-elwell January 10, 2024 08:58
@g-elwell
Copy link
Member

I've changed the test as per our conversation yesterday and resolved the conflict

Cool! I think that since we're actually testing the toolbar version number displays correctly, and not really whether or not you are able to delete a post, that means the PR info should be updated and code-wise the test descriptions and filename will need to change too.

@johnmunster johnmunster changed the title [4] delete release notes test [4] Version control change after release note deletion Jan 10, 2024
@johnmunster
Copy link
Contributor Author

Updated necessary details as per previous comment.

Copy link
Member

@g-elwell g-elwell left a comment

Choose a reason for hiding this comment

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

Looks good to me, test passing locally. Love the new CLI command too!

image

@johnmunster johnmunster merged commit ccc97cd into main Jan 10, 2024
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.

2 participants