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

Simplify i18n tests and remove sh.sed usage #6562

Conversation

nabla-c0d3
Copy link
Contributor

@nabla-c0d3 nabla-c0d3 commented Sep 24, 2022

Status

Ready

Description of Changes

  • Simplify i18n-related tests:
    • De-couple the tests from the journalist_app and config fixtures, which are not actually needed by the tests.
    • Get closer to implementing an immutable SDConfig.
  • Remove usage of sh.sed (Remove sh dependency #6547).

This PR should be reviewed and merged after #6551.

@nabla-c0d3 nabla-c0d3 mentioned this pull request Sep 24, 2022
@nabla-c0d3 nabla-c0d3 force-pushed the simplify-i18n-tests-and-remove-sed-usage branch from 175475e to 4c05629 Compare September 24, 2022 10:12
@lgtm-com
Copy link

lgtm-com bot commented Sep 24, 2022

This pull request introduces 2 alerts when merging 4c05629 into ce7077d - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

@nabla-c0d3 nabla-c0d3 force-pushed the simplify-i18n-tests-and-remove-sed-usage branch from 4c05629 to b0adc04 Compare September 24, 2022 10:29
@lgtm-com
Copy link

lgtm-com bot commented Sep 24, 2022

This pull request introduces 2 alerts when merging b0adc04 into ce7077d - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Overall looks good!

securedrop/tests/test_journalist.py Outdated Show resolved Hide resolved
@legoktm legoktm added this to the 2.6.0 milestone Sep 27, 2022
@nabla-c0d3 nabla-c0d3 force-pushed the simplify-i18n-tests-and-remove-sed-usage branch from b0adc04 to b420f79 Compare September 28, 2022 19:29
@lgtm-com
Copy link

lgtm-com bot commented Sep 28, 2022

This pull request introduces 2 alerts when merging b420f79 into cc551cf - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

@nabla-c0d3 nabla-c0d3 force-pushed the simplify-i18n-tests-and-remove-sed-usage branch from b420f79 to ed2f848 Compare September 28, 2022 19:58
@lgtm-com
Copy link

lgtm-com bot commented Sep 28, 2022

This pull request introduces 2 alerts when merging ed2f848 into cc551cf - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a call

@nabla-c0d3 nabla-c0d3 force-pushed the simplify-i18n-tests-and-remove-sed-usage branch from ed2f848 to eb9ef64 Compare September 30, 2022 18:28
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

This is really great!

@legoktm legoktm merged commit ce83e85 into freedomofpress:develop Sep 30, 2022
@nabla-c0d3
Copy link
Contributor Author

Thanks!

@nabla-c0d3 nabla-c0d3 deleted the simplify-i18n-tests-and-remove-sed-usage branch October 1, 2022 07:52
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.

None yet

2 participants