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 deprecation warnings when running tests #1899

Merged
merged 4 commits into from
Feb 15, 2021

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Feb 1, 2021

What / why

Running the tests for the gem locally some deprecation warnings were being thrown up. They were:

  • action_view.raise_on_missing_translations is deprecated
  • rendering a template with a . in it is deprecated

The first one was an easy fix, just swapping out the old for the new. The second one was a little more complicated. The attachment component has several partials that are .svg files but are rendered like partials. I renamed these files to .html.erb and removed the file extension from the render call, which removed the deprecation warning but suddenly caused one of the component audit tests to fail.

This was because the formerly .svg files were now being recognised by the auditor as potential components that should be included when suggesting Sass/JS for applications, even though they were in a subdirectory for partials and not the main directory location for component templates. The fix for this was to amend the regex that matches this to remove the / character when matching components. So if this was a template...

<%= render "govuk_publishing_components/components/component" %>
<%= render "govuk_publishing_components/components/partials/partial" %>

Previously the auditor was identifying both lines as components, when only the first line is a true component. The question I'm left with is why the regex originally included the / character when matching... it seems a bit odd. I've checked it locally with collections and it seems to still produce the right suggested Sass/JS output, but perhaps some more thorough checking would be useful.

I think this only really happened with components within components but I think one of the publishing apps might pull in a partial from the header component directly for some reason, I'm hoping someone can shed some light on this.

The specific deprecation warnings were:

DEPRECATION WARNING: action_view.raise_on_missing_translations is deprecated and will be removed in Rails 6.2. 
Set i18n.raise_on_missing_translations instead. Note that this new setting also affects how missing translations are handled in controllers. 
(called from block (2 levels) in <top (required)> at /Users/andysellick/workspace/govuk/govuk_publishing_components/spec/component_guide/component_audit_spec.rb:5)

DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: 
govuk_publishing_components/components/attachment/_thumbnail_spreadsheet.svg 
(called from _app_views_layouts_application_html_erb__3522064166250592246_71200 at /Users/andysellick/workspace/govuk/govuk_publishing_components/spec/dummy/app/views/layouts/application.html.erb:16)

DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: 
govuk_publishing_components/components/attachment/_thumbnail_spreadsheet.svg 
(called from _app_views_layouts_application_html_erb__3522064166250592246_71200 at /Users/andysellick/workspace/govuk/govuk_publishing_components/spec/dummy/app/views/layouts/application.html.erb:16)

DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: 
govuk_publishing_components/components/attachment/_thumbnail_generic.svg 
(called from block (3 levels) in ___sers_andysellick_workspace_govuk_govuk_publishing_components_app_views_govuk_publishing_components_components__attachment_html_erb___2448659267357681147_74020 at /Users/andysellick/workspace/govuk/govuk_publishing_components/app/views/govuk_publishing_components/components/_attachment.html.erb:48)

DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: 
govuk_publishing_components/components/attachment/_thumbnail_generic.svg 
(called from block (3 levels) in ___sers_andysellick_workspace_govuk_govuk_publishing_components_app_views_govuk_publishing_components_components__attachment_html_erb___2448659267357681147_74020 at /Users/andysellick/workspace/govuk/govuk_publishing_components/app/views/govuk_publishing_components/components/_attachment.html.erb:48)

DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: 
govuk_publishing_components/components/attachment/_thumbnail_document.svg 
(called from block (3 levels) in ___sers_andysellick_workspace_govuk_govuk_publishing_components_app_views_govuk_publishing_components_components__attachment_html_erb___2448659267357681147_74020 at /Users/andysellick/workspace/govuk/govuk_publishing_components/app/views/govuk_publishing_components/components/_attachment.html.erb:44)

DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: 
govuk_publishing_components/components/attachment/_thumbnail_document.svg 
(called from block (3 levels) in ___sers_andysellick_workspace_govuk_govuk_publishing_components_app_views_govuk_publishing_components_components__attachment_html_erb___2448659267357681147_74020 at /Users/andysellick/workspace/govuk/govuk_publishing_components/app/views/govuk_publishing_components/components/_attachment.html.erb:44)

DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: 
govuk_publishing_components/components/attachment/_thumbnail_spreadsheet.svg 
(called from block (3 levels) in ___sers_andysellick_workspace_govuk_govuk_publishing_components_app_views_govuk_publishing_components_components__attachment_html_erb___2448659267357681147_74020 at /Users/andysellick/workspace/govuk/govuk_publishing_components/app/views/govuk_publishing_components/components/_attachment.html.erb:46)

DEPRECATION WARNING: Rendering actions with '.' in the name is deprecated: 
govuk_publishing_components/components/attachment/_thumbnail_spreadsheet.svg 
(called from block (3 levels) in ___sers_andysellick_workspace_govuk_govuk_publishing_components_app_views_govuk_publishing_components_components__attachment_html_erb___2448659267357681147_74020 at /Users/andysellick/workspace/govuk/govuk_publishing_components/app/views/govuk_publishing_components/components/_attachment.html.erb:46)

Visual Changes

None.

@bevanloon bevanloon temporarily deployed to govuk-publis-fix-deprec-uic6av February 1, 2021 10:24 Inactive
@andysellick andysellick force-pushed the fix-deprecation-warnings branch from e403b63 to c1bd687 Compare February 1, 2021 12:22
@andysellick andysellick marked this pull request as ready for review February 1, 2021 12:40
- following the change to remove '.' from component render calls, specifically the /attachment/_thumbnail ... .svg files, the component auditing tests were failing because it was now detecting those files renamed from .svg to .html.erb as components and including them as things that should be included in the component audit results
- modifying the code to remove the inclusion of the '/' character when detecting the component name fixes this problem, but raises the question why it was there in the first place, as all of our components should be in the top level dir and therefore never include a '/' character
@andysellick andysellick force-pushed the fix-deprecation-warnings branch from c1bd687 to 23687fc Compare February 15, 2021 08:35
@andysellick andysellick merged commit 4af2c16 into master Feb 15, 2021
@andysellick andysellick deleted the fix-deprecation-warnings branch February 15, 2021 08:46
alex-ju added a commit that referenced this pull request Feb 15, 2021
## 24.1.1

* Fix deprecation warnings when running tests ([PR #1899](#1899))
* Update `govuk-frontend` base SCSS imports ([PR #1922](#1922))
* Remove redundant import in accordion component ([PR #1923](#1923))
* Fix toggle click tracking on step-by-steps ([PR #1925](#1925))
* Accordion summary design adjustment ([PR #1926](#1926))
* Fix `layout_header` layout and spacing issues ([PR #1924](#1924))
@alex-ju alex-ju mentioned this pull request Feb 15, 2021
This was referenced Mar 11, 2021
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.

3 participants