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

📁 Feed: Splitting Tests Into Separated Files #64

Merged
merged 7 commits into from
Apr 12, 2022

Conversation

paOmer
Copy link
Collaborator

@paOmer paOmer commented Apr 3, 2022

This PR aims to take the existing feed app tests and split them into separated files.
In future sight, many more tests will be added, so to remain organized it is best to split big files into small and logically separated files.

  • Deleted old tests.py file
  • Added a new tests directory inside the feed app root directory
  • Added different tests file which contain the previous tests
  • Changed tests function names to be more meaningful

Close #66

@paOmer paOmer changed the title 🏇 Feed: Splitting Tests Into Separated Files 📁 Feed: Splitting Tests Into Separated Files Apr 3, 2022
@paOmer paOmer self-assigned this Apr 3, 2022
Copy link
Collaborator

@DeanBiton DeanBiton left a comment

Choose a reason for hiding this comment

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

All tests seem to be included.
Good job on improving the names.

Copy link
Collaborator

@taljacob2 taljacob2 left a comment

Choose a reason for hiding this comment

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

LGTM!
Great initiative! 👍

Copy link
Collaborator

@ShirleyFichman ShirleyFichman left a comment

Choose a reason for hiding this comment

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

Good idea, well done.

def test_comment_save_to_db(self, comments):
# Testing comment save in DB
comments[0].save()
assert comments[0] in Comment.comments.all()
Copy link
Contributor

@Yarboa Yarboa Apr 5, 2022

Choose a reason for hiding this comment

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

same here, the tests should filter the comment in db, and compare it against the returned data from fixtures

Please apply for all the rest of the tests

Copy link
Collaborator Author

@paOmer paOmer Apr 7, 2022

Choose a reason for hiding this comment

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

Hi @Yarboa,
In this case I try to check if the model really got saved in DB, so I believe this is the correct way to do so.
If you're still think that a change is required here, will the change look like the following? :

def test_comment_save_to_db(self, comments):
         # Testing comment save in DB
         comments[0].save()
         comment = Comment.comments.filter(content=COMMENT1_CONTENT).first()
         assert comment in Comment.comments.all()

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but please compare it with all comments data

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Im not sure what do you mean by compare to all comments data, do you mean to filter according to all properties and not only to content?

Copy link
Contributor

@Yarboa Yarboa Apr 12, 2022

Choose a reason for hiding this comment

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

This is the purpose of the test,
If you have a comment, how do you find the logical connections from db, FYI

@paOmer paOmer force-pushed the feed_split_tests branch from 3de23a6 to e6335ef Compare April 5, 2022 19:13
@Yarboa
Copy link
Contributor

Yarboa commented Apr 7, 2022

@paOmer please take a look at previous comments

@paOmer paOmer marked this pull request as draft April 7, 2022 14:49
@paOmer paOmer marked this pull request as ready for review April 7, 2022 16:27
@paOmer paOmer mentioned this pull request Apr 8, 2022
@paOmer paOmer force-pushed the feed_split_tests branch from f17b89e to 2abf060 Compare April 8, 2022 14:55
@Yarboa Yarboa self-requested a review April 10, 2022 08:40
def test_comment_save_to_db(self, comments):
# Testing comment save in DB
comments[0].save()
assert comments[0] in Comment.comments.all()
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but please compare it with all comments data

@paOmer
Copy link
Collaborator Author

paOmer commented Apr 10, 2022

Hi @Yarboa, after you'll review I would squash and force push, as mentioned in the last Beyond session.

@paOmer paOmer requested a review from Yarboa April 10, 2022 11:16
Copy link
Contributor

@Yarboa Yarboa left a comment

Choose a reason for hiding this comment

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

LGTM

@Yarboa Yarboa merged commit 0ff831a into redhat-beyond:main Apr 12, 2022
@paOmer paOmer deleted the feed_split_tests branch April 12, 2022 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📁 Feed: Splitting Tests File
5 participants