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.

Sign up for GitHub

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

Jump to bottom

[System Tests] Post Full Page, Then Immediately Comment #8879

Merged
merged 3 commits into from
Jan 6, 2021
Merged

[System Tests] Post Full Page, Then Immediately Comment #8879

merged 3 commits into from
Jan 6, 2021

Conversation

noi5e
Copy link
Contributor

@noi5e noi5e commented Dec 21, 2020 •
edited
Loading

Merge this PR first! (I have another one open at #8881)

  • Also made new helper function to retrieve node path in the comment tests.

(This PR is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

@noi5e noi5e added testing issues are usually for adding unit tests, integration tests or any other tests for a feature outreachy labels Dec 21, 2020
@gitpod-io
Copy link

gitpod-io bot commented Dec 21, 2020 •
edited
Loading

@codecov
Copy link

codecov bot commented Dec 21, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@0f770f2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8879   +/-   ##
=======================================
  Coverage        ?   81.92%           
=======================================
  Files           ?      100           
  Lines           ?     5948           
  Branches        ?        0           
=======================================
  Hits            ?     4873           
  Misses          ?     1075           
  Partials        ?        0

This was referenced Dec 22, 2020
Merged
Closed
Sagarpreet
Sagarpreet reviewed Dec 22, 2020
View reviewed changes
test/system/comment_test.rb Outdated
page.evaluate_script("addComment('#{comment_text}', '/comment/create/#{nodes(node_name).nid.to_s}')")
assert_selector('#comments-list .comment-body p', text: comment_text)
end

test "#{page_type_string}: reply to existing comment" do
node_name == :wiki_page ? (visit nodes(node_name).path + '/comments') : (visit nodes(node_name).path)
visit get_visit_path(node_name, nodes(node_name).path)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is repeated 2 times, is it intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Sagarpreet! Which part are you thinking about?

  1. visit get_visit_path looks a little redundant and confusing, maybe I should rename the method to something like get_path or get_path_string.
  • visit is the testing method that visits the page
  • get_visit_path is a helper function I wrote to retrieve the path for the testing node. mainly because wiki comments are only viewable at /wiki/wiki-name/comments. (i have to add '/comments' to the end of the path string)
  1. there were two addComment tests that existed before I was an intern. for some reason, there are two tests for this function. one tests addComment(text, URL), the other just addComment(text).
  • I was thinking that maybe these tests aren't necessary, aren't really 'system' tests, and could be removed.

Copy link
Member

Choose a reason for hiding this comment

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

I like get_path here, thanks!

If the 2 old tests are redundant and you're confident we test them in other ways, that's fine. Are they not system tests because they test only the functions and not the full-stack interaction? I think there may be an ambiguity because there wasn't another way to test JS functions, and these would address both JS and Ruby, so they are "mostly-full-stack" still... what do you think?

My apologies for missing this PR i see it's been open a while. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

We can merge before or after changing to get_path and if you'd still like to eliminate the old tests we can do that in another PR, perhaps? Thank you!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to get_path...

Re: addComment, that's a good point, I guess the JS wouldn't be tested in any other way besides here. They're also pretty lightweight and fast-running (compared to mine 😛).

It seems like addComment isn't used anywhere on the site besides in these tests... Yet! I guess that'll be one of the things I do, to try to implement it in most comment forms (it's on my to-dos in the planning issue)... I think once that goes live, then these tests could be removed, because the the manual commenting tests would run via addComment.

test/system/comment_test.rb
@@ -80,8 +85,47 @@ def setup
find("p", text: comment_response_text)
end

test "post #{page_type_string}, then comment on FRESH #{page_type_string}" do
title_text, body_text = String.new, String.new
case page_type_string
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is really cool !!
How is page_type_string getting substituted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's happening here:

plots2/test/test_helper.rb

Lines 43 to 50 in 9fb5d65

# used in comment_test.rb
def page_types
{
:note => :comment_note,
:question => :comment_question,
:wiki => :wiki_page
}
end

Helper function that returns the name of the node fixture :)

@noi5e noi5e changed the title New Test: Post, then Comment on Fresh Post Testing: Post Full Page, Then Immediately Comment Dec 27, 2020
@noi5e
Copy link
Contributor Author

noi5e commented Jan 6, 2021

Ready to merge!

@jywarren
Copy link
Member

jywarren commented Jan 6, 2021

Awesome! Just one conflict to resolve!!

@noi5e
Copy link
Contributor Author

noi5e commented Jan 6, 2021

Resolved!

@codeclimate
Copy link

codeclimate bot commented Jan 6, 2021

Code Climate has analyzed commit 9b5632a and detected 0 issues on this pull request.

View more on Code Climate.

@jywarren jywarren merged commit 0f4dec9 into publiclab:main Jan 6, 2021
@jywarren
Copy link
Member

jywarren commented Jan 6, 2021

Fantastic!!!!

@noi5e noi5e deleted the test/post-then-comment branch January 6, 2021 04:39
@noi5e noi5e mentioned this pull request Jan 23, 2021
Open
@noi5e noi5e changed the title Testing: Post Full Page, Then Immediately Comment [System Tests] Post Full Page, Then Immediately Comment Jan 23, 2021
manchere pushed a commit to manchere/plots2 that referenced this pull request Feb 13, 2021
@noi5e @manchere
Testing: Post Full Page, Then Immediately Comment (publiclab#8879)
b5c2db9 
* new test: post fresh page, then comment on it

* refactored get_path method for readability
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this pull request Mar 2, 2021
@noi5e @lagunasmel
Testing: Post Full Page, Then Immediately Comment (publiclab#8879)
933ab71 
* new test: post fresh page, then comment on it

* refactored get_path method for readability
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this pull request Oct 16, 2021
@noi5e @reginaalyssa
Testing: Post Full Page, Then Immediately Comment (publiclab#8879)
cffaac8 
* new test: post fresh page, then comment on it

* refactored get_path method for readability
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this pull request Dec 28, 2021
@noi5e @billymoroney1
Testing: Post Full Page, Then Immediately Comment (publiclab#8879)
229dc94 
* new test: post fresh page, then comment on it

* refactored get_path method for readability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sagarpreet Sagarpreet Sagarpreet left review comments

@jywarren jywarren jywarren approved these changes

@sagarpreet-chadha sagarpreet-chadha Awaiting requested review from sagarpreet-chadha

@cesswairimu cesswairimu Awaiting requested review from cesswairimu

Assignees
No one assigned
Labels
outreachy testing issues are usually for adding unit tests, integration tests or any other tests for a feature
Projects
None yet
Milestone
[Outreachy] Comment Editor Winter 2020
Development

Successfully merging this pull request may close these issues.
3 participants
@noi5e @jywarren @Sagarpreet