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

Fix intermittent failing tests #1057

Merged
merged 5 commits into from
Aug 17, 2018
Merged

Conversation

oscarwyatt
Copy link
Contributor

@oscarwyatt oscarwyatt commented Aug 16, 2018

Improves further on PR #1052. Moved custom configuration to a before_initialize block which seems to solve a problem with the config not being available in test environment. Based on the following stackoverflow post

https://stackoverflow.com/a/44770663

screenshot-stackoverflow com-2018 08 16-14-06-26

Trello: https://trello.com/c/toblYkmD/133-investigate-and-fix-failing-tests-on-government-frontend


Visual regression results:
https://government-frontend-pr-1057.surge.sh/gallery.html

Component guide for this PR:
https://government-frontend-pr-1057.herokuapp.com/component-guide

@benthorner benthorner temporarily deployed to government-frontend-pr-1057 August 16, 2018 12:08 Inactive
@benthorner benthorner temporarily deployed to government-frontend-pr-1057 August 16, 2018 12:48 Inactive
@oscarwyatt oscarwyatt changed the title Extend wait time for failing test [WIP] Extend wait time for failing test Aug 16, 2018
@oscarwyatt oscarwyatt changed the title [WIP] Extend wait time for failing test Extend wait time for failing test Aug 16, 2018
@vanitabarrett
Copy link
Contributor

@oscarwyatt Not sure this is resolved - still getting the same errors, e.g:

[build] Failure:
[build] LinksOutConfigTest#test_links_out_configuration_causes_no_errors_and_correct_supergroups_are_displayed_for_each_ruleset [/var/lib/jenkins/workspace/intermittently-failing-test-IAOZESY5I7JDI3LIV3SCFYLXHQ4CKM4DHPPJL6OA4VBZIDUKB4CA/test/integration/links_out_config_test.rb:60]:
[build] Expected false to be truthy.

and

[build] Error:
[build] LinksOutConfigTest#test_links_out_configuration_causes_no_errors_and_correct_supergroups_are_displayed_for_each_ruleset:
[build] NoMethodError: undefined method `each_key' for nil:NilClass
[build]     test/integration/links_out_config_test.rb:56:in `block (3 levels) in <class:LinksOutConfigTest>'
[build]     test/integration/links_out_config_test.rb:55:in `each_key'
[build]     test/integration/links_out_config_test.rb:55:in `block (2 levels) in <class:LinksOutConfigTest>'
[build]     test/integration/links_out_config_test.rb:54:in `block in <class:LinksOutConfigTest>'
[build] 
[build] bin/rails test test/integration/links_out_config_test.rb:51

I've been taking a look this afternoon too, but haven't managed to make any progress on it. I think if we still don't have a solution tomorrow we should probably remove this test for now so we're not slowing other people down in government-frontend (as it's updated quite often). We can add it back in when we're confident it's working 🙂

@oscarwyatt
Copy link
Contributor Author

@vanitabarrett I think you're probably right.
According to the rails source, there are some situations in which config_for will return a blank hash
`# File railties/lib/rails/application.rb, line 227
def config_for(name)
yaml = Pathname.new("#{paths["config"].existent.first}/#{name}.yml")

  if yaml.exist?
    require "erb"
    (YAML.load(ERB.new(yaml.read).result) || {})[Rails.env] || {}
  else
    raise "Could not load configuration. No such file - #{yaml}"
  end
rescue Psych::SyntaxError => e
  raise "YAML syntax error occurred while parsing #{yaml}. "          "Please note that YAML must be consistently indented using spaces. Tabs are not allowed. "          "Error: #{e.message}"
end`

Although it's not exactly clear why this is happening in CI. I've made an amendment to load the file directly from source, which isn't ideal as someone could come along and change which file is used. If this still isn't working I'm happy for you to get rid of it and I'll try and think of a better way of testing the config file

andrewgarner
andrewgarner previously approved these changes Aug 17, 2018
@vanitabarrett vanitabarrett changed the title Extend wait time for failing test [WIP][DO NOT MERGE] Extend wait time for failing test Aug 17, 2018
@benthorner benthorner temporarily deployed to government-frontend-pr-1057 August 17, 2018 10:25 Inactive
Dynamically generate the tests, not the assertions based on the links
out config. This will allow us to see which test is failing and
decreases the chance of a test timing out.
@benthorner benthorner temporarily deployed to government-frontend-pr-1057 August 17, 2018 12:44 Inactive
Let's see if the approach set out in the Rails documentation works now
that we have refactored related tests.
@andrewgarner
Copy link
Contributor

andrewgarner commented Aug 17, 2018

@vanitabarrett @oscarwyatt I've made some changes that I hope will help this:

  1. I've made it so we actually stub the Rails configuration object not modify it under test. This is to ensure we don't accidentally break the application which I think could have happened with randomly ordered tests.

  2. I've rewritten the config test so that the tests are dynamically generated based on the contents of the config file. This means that each test only has to make one browser request and a couple of assertions. Hopefully that makes timeouts less likely. Hopefully it is also a bit easier to comprehend.

@andrewgarner andrewgarner dismissed their stale review August 17, 2018 13:14

I've added alternative commits

@andrewgarner andrewgarner changed the title [WIP][DO NOT MERGE] Extend wait time for failing test Fix intermittent failing tests Aug 17, 2018
Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Choose a reason for hiding this comment

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

Changes look good 👍 Rebuilt a few times and all passed too

@andrewgarner andrewgarner merged commit 3c7fcd5 into master Aug 17, 2018
@andrewgarner andrewgarner deleted the fix-intermittently-failing-test branch August 17, 2018 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants