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

Populate LevelConceptDifficulty fixture to fix assert_nil warning #13748

Merged
merged 4 commits into from
Mar 14, 2017

Conversation

joshlory
Copy link
Contributor

Before adding this fixture, "level_concept_difficulty uses preloading" in ScriptTest only compared nil to nil.

Fixes:

Use assert_nil if expecting nil from /home/ubuntu/code-dot-org/dashboard/test/models/script_test.rb:346:in `block in <class:ScriptTest>'. This will fail in MT6.
  test_level_concept_difficulty_uses_preloading                   PASS (0.08s)

Before adding this fixture, "level_concept_difficulty uses preloading" in `ScriptTest` only compared nil to nil.
@ashercodeorg
Copy link
Contributor

ashercodeorg commented Mar 13, 2017

AFAICT (though I'm not 100% certain), this change shouldn't be necessary. Presumably, if the DB is disconnected, the call to level_concept_difficulty should blow up unless preloading occurred (regardless of whether the level_concept_difficulty is nil or non-nil? If so, I would suggest simplifying the test to use assert_nil (removing expected).

@Hamms
Copy link
Contributor

Hamms commented Mar 13, 2017

Oh dang, that's an unfortunate oversight.

What we probably actually want to do is add an assert_not_nil somewhere before line 344

@joshlory
Copy link
Contributor Author

@ashercodeorg I'm not sure I follow. This is the code for the test:

test 'level_concept_difficulty uses preloading' do
  level = Script.find_by_name('course4').script_levels.second.level
  expected = level.level_concept_difficulty

  populate_cache_and_disconnect_db

  assert_equal expected, level.level_concept_difficulty
end

Without this change, Script.find_by_name('course4').script_levels.second.level has no level_concept_difficulty.

@ashercodeorg
Copy link
Contributor

Right. But you need the DB to know that it has no level_concept_difficulty. It isn't clear to me that the test isn't successfully testing what it wants to test, regardless of whether the level_concept_difficulty exists.

@joshlory
Copy link
Contributor Author

Ah, I see what you're saying. Added an assert_not_nil in cbd9b38. PTAL!

@ashercodeorg
Copy link
Contributor

LGTM.

@joshlory joshlory merged commit ed9f706 into staging Mar 14, 2017
@joshlory joshlory deleted the fix-assert-nil-concept-difficulty-test branch March 14, 2017 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants