15/07/22 #236
15/07/22 #236
Conversation
|
|
||
| class Peep | ||
| def self.all | ||
| if ENV['ENVIRONMENT'] == 'test' | ||
| connection = PG.connect(dbname: 'chitter_test') | ||
| else | ||
| connection = PG.connect(dbname: 'chitter') | ||
| end | ||
|
|
||
| result = connection.exec("SELECT * FROM peeps") | ||
| result.map { |peep| peep['message'] } | ||
| end | ||
|
|
||
| def self.create(peep:, timestamp:) | ||
| if ENV['ENVIRONMENT'] == 'test' | ||
| connection = PG.connect(dbname: 'chitter_test') | ||
| else | ||
| connection = PG.connect(dbname: 'chitter') | ||
| end | ||
|
|
||
| connection.exec("INSERT INTO peeps (message, timestamp) VALUES('#{peep}', #{timestamp})") |
| @@ -0,0 +1,8 @@ | |||
| feature 'Viewing peeps' do | |||
| scenario 'viewing related timestamp' do | |||
There was a problem hiding this comment.
It seems like this test is just for viewing the peeps rather than timestamps, as the expected outcome is just a lil peep peep. I'm guessing you were halfway through adapting an existing test?
There was a problem hiding this comment.
Nice observation @Amy-O ! I agree, right now this test is not testing what it says it does.
| # connection = PG.connect(dbname: 'chitter_test') | ||
|
|
||
| # # Add the test data | ||
| # connection.exec("INSERT INTO peeps VALUES(1, 'Peep one');") | ||
| # connection.exec("INSERT INTO peeps VALUES(2, 'Peep two');") | ||
| # connection.exec("INSERT INTO peeps VALUES(3, 'Peep three');") | ||
|
|
||
| # Bookmark.create(website: "http://www.makersacademy.com") | ||
| # Bookmark.create(website: "http://www.destroyallsoftware.com") | ||
| # Bookmark.create(website: "http://www.google.com") |
There was a problem hiding this comment.
I didn't think to add the bookmark stuff in comments for easier comparison! smart
|
|
||
| get '/peeps' do | ||
| @peeps = Peep.all | ||
| # @time = Peep.time |
There was a problem hiding this comment.
It's always a good idea to remove commented out code from code you submit for code review (anywhere, not just at Makers)
| @@ -0,0 +1,2 @@ | |||
| -- To both chitter and chitter_test databases: | |||
| ALTER TABLE peeps ADD COLUMN timestamp VARCHAR(60); | |||
There was a problem hiding this comment.
Nice use of the timestamp column type!
| if ENV['ENVIRONMENT'] == 'test' | ||
| connection = PG.connect(dbname: 'chitter_test') | ||
| else | ||
| connection = PG.connect(dbname: 'chitter') | ||
| end |
There was a problem hiding this comment.
You're using this same piece of code in more than one place in this file. How might you get rid of this duplication?
| @@ -0,0 +1,8 @@ | |||
| feature 'Viewing peeps' do | |||
| scenario 'viewing related timestamp' do | |||
There was a problem hiding this comment.
Nice observation @Amy-O ! I agree, right now this test is not testing what it says it does.
| visit('/peeps/add') | ||
| fill_in('peep', with: 'Peep peep!') | ||
| click_button('Post') | ||
| expect(page).to have_content "" |
There was a problem hiding this comment.
Shouldn't we be expecting the page to have "Peep peep!" as content?
| @@ -41,6 +42,11 @@ | |||
| RSpec.configure do |config| | |||
| config.before(:each) do | |||
| setup_test_database | |||
|
|
|||
| # new_time = Time.local(2008, 9, 1, 12, 0, 0) | |||
| # Timecop.freeze(new_time) | |||
There was a problem hiding this comment.
I see you were trying to use Timecop to freeze the time during tests. Maybe it was tricky getting it to work? An alternative that doesn't require an additional gem is to use mocking, e.g.:
@now = Time.now
allow(Time).to receive(:now).and_return(@now)
| end | ||
|
|
||
| post '/peeps' do | ||
| Peep.create(peep: params[:peep], timestamp: params[:timestamp]) |
There was a problem hiding this comment.
Passing the time in here is one way to do it but it's vulnerable to bugs. For example:
- the user may click "submit" some time after the page was generated from your
erbview, so the time of the peep will be inaccurate - the user can influence the time by sending their own POST requests and putting in a bogus time (e.g. to get their peep to show first even though it's old)
For such reasons, it's best to generate timestamps in your Model when you're about to add the peep to the database so that you're sure it will be accurate. I see that you were thinking along those lines in your commented out code in the chitter.rb. You were on the right track!
To control the time in tests, you can stub the return value of Time.now (see another comment further down).
| @@ -0,0 +1,5 @@ | |||
| <ul> | |||
| <% @peeps.each do |peep| %> | |||
| <li><%=peep%></li> | |||
There was a problem hiding this comment.
All the rest of your code is there to fulfil user story 3 (showing the timestamp) -- did you run out of time or did you get blocked?
Your name
Emily Bayliss
Please write your full name here to make it easier to find your pull request.
User stories
Please list which user stories you've implemented (delete the ones that don't apply).
README checklist
Does your README contains instructions for
Here is a pill that can help you write a great README!