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
Refactor contact rollups code to library, add tests #13388
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
require 'test_helper' | ||
require 'cdo/contact_rollups' | ||
|
||
PEGASUS_TEST_DB_NAME = "pegasus_#{Rails.env}" | ||
COLUMN_NAME_TO_INDEX_MAP = { "roles": 0, "ages_taught": 1 }.freeze | ||
|
||
class PardotTest < ActiveSupport::TestCase | ||
def test_empty_contacts | ||
# Test the rollup process with an empty database | ||
rollups_test_helper 0, [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even with the comment, I still have no idea what the expectations are. Part of this is the
|
||
end | ||
|
||
def test_teachers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be split into multiple tests, suggested by the fact that the test can fail for any number of reasons. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the rollup process operates on a large corpus of data, I am going to leave this as is. If I split it up, each test would be operating on a database with a small number of users (such as one). By generating a mildly interesting body of test fixtures, it demonstrates that we don't have something going on such as bad joins that will generate additional rows in the output. Things like that might not show up in the smaller data set of the individual things we are verifying later. |
||
# Create teacher 1 with no section | ||
@teacher1 = create(:teacher, email: "rolluptestteacher1@code.org") | ||
|
||
# Create teacher 2 with one section and one student | ||
@teacher2 = create(:teacher, email: "rolluptestteacher2@code.org") | ||
create_sections_helper @teacher2, [[{age: 6}]] | ||
|
||
# Create teacher 3 with one section and multiple students | ||
@teacher3 = create(:teacher, email: "rolluptestteacher3@code.org") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These don't need to be instance variables. In fact, they don't even need to be stored in variables at all since they are never referenced. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thx |
||
@teacher3_section = create(:section, user: @teacher3) | ||
create_sections_helper @teacher3, [[{age: 6}, {age: 10}, {age: 14}, {age: 10}]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the section on line 23 for? Line 24 will create a new section inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps to test a teacher with a section with no students? At least, I see that as being a useful test to have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be a useful test, and it would make more sense to have a separate teacher with an empty section. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was in there accidentally. But it is a good test to have an empty section, I added that in deliberately now. |
||
|
||
# Create teacher 4 with three sections and multiple students | ||
@teacher4 = create(:teacher, email: "rolluptestteacher4@code.org") | ||
create_sections_helper @teacher4, | ||
[ | ||
[{age: 14}, {age: 10}, {age: 11}, {age: 10}, {age: 10}, {age: 11}], | ||
[{age: 9}, {age: 11}, {age: 9}], | ||
[{age: 15}] | ||
] | ||
|
||
# Expected values for ages taught is the union of all unique values of student ages, | ||
# no duplicates, in ascending sorted order | ||
expected_values = [ | ||
{ "rolluptestteacher1@code.org": { "roles": "Teacher", "ages_taught": nil }}, | ||
{ "rolluptestteacher2@code.org": { "roles": "Teacher", "ages_taught": "6" }}, | ||
{ "rolluptestteacher3@code.org": { "roles": "Teacher", "ages_taught": "6,10,14" }}, | ||
{ "rolluptestteacher4@code.org": { "roles": "Teacher", "ages_taught": "9,10,11,14,15" }} | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice :) |
||
|
||
rollups_test_helper 4, expected_values | ||
end | ||
|
||
private | ||
|
||
def rollups_test_helper(expected_count, expected_values) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method name is pretty generic. Since it builds the contact rollups and asserts a bunch of values, perhaps rename something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, thx. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please provide YARD comments for helper methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thx |
||
# Run the rollup process | ||
ContactRollups.build_contact_rollups | ||
|
||
# Should now have expected_count records in daily rollups table, and still none in main rollups table | ||
assert ActiveRecord::Base.connection.execute("select count(*) from pegasus_test.contact_rollups_daily").first[0] == expected_count | ||
assert ActiveRecord::Base.connection.execute("select count(*) from pegasus_test.contact_rollups").first[0] == 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use assert_equal. Note the expected value comes first, so: assert_equal expected_count, ActiveRecord::Base... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thx (throughout) |
||
|
||
# Verify expected values in contacts_rollup_daily | ||
expected_values.each do |expected_values_hash| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That this is so unreadable suggests the
and
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aside: This (as written and as suggested) runs a DB query per email. It might make sense to optimize the test to grab all the results, then compare all the results. Depending, it might make sense to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea on the simplification, done, thx. On the test run time optimization - trying to batch the 5 DB queries into 1 only saves a handful of millisecond of test run time, not worth doing. |
||
email = expected_values_hash.keys.first | ||
email_sanitized = ActiveRecord::Base.sanitize(expected_values_hash.keys.first) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose it can't hurt, but I don't think you need to sanitize values that you provide in the test... unless you're worried about your own SQL injection attack ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can hurt, because it obfuscates what is happening in the test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was doing this on general principle. Since you both had negative reactions to it, I have pulled it out. Once we start running static security code analysis routinely it will presumably light up on this and I may have to do something. |
||
results = ActiveRecord::Base.connection.execute("select roles, ages_taught from pegasus_test.contact_rollups_daily where email=#{email_sanitized}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a big deal, but it would be easier to read if this def execute(sql)
ActiveRecord::Base.connection.execute(sql)
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disagree. :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ashercodeorg out of curiosity, why not? It could be better named, such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could go either way. There is cost to the code reader to delve into the aliased function to see what it does. Since we don't do this routinely/consistently elsewhere, I am going to leave as is. |
||
assert results.count == 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thx |
||
row = results.first | ||
expected_column_values = expected_values_hash[email] | ||
expected_column_values.each do |key, value| | ||
column_index = COLUMN_NAME_TO_INDEX_MAP[key] | ||
# puts ("email: #{email} actual: #{row[column_index]} expected: #{value}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove commented-out line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thx |
||
assert row[column_index] == value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assert_equal |
||
end | ||
end | ||
end | ||
|
||
def create_sections_helper(teacher, sections) | ||
sections.each do |students| | ||
@section = create(:section, user: teacher) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will reassign the instance variable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thx |
||
students.each do |student| | ||
create_follower_helper @section, student[:age] | ||
end | ||
end | ||
end | ||
|
||
def create_follower_helper(section, age) | ||
# subtract an additional day when calculating birthday for desired age to avoid time zone issues | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead, you can set the test to always think it's running at a specific time. Just add freeze_time to the beginning of you test class. See example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, thx |
||
@student = create(:student, birthday: Time.zone.today - age.years - 1.days) | ||
create(:follower, section: section, student_user: @student) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quotes around
roles
andages_taught
are superfluous.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thx