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

(PDK-1167) Validator should honor case sensitive of the file system #646

Merged
merged 3 commits into from
Apr 2, 2019

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Mar 29, 2019

Blocked on fb12b97 being merged


Previously the validator expected that any targets passed into the parse_targets
method were always case sensitive due to the way File.fnmatch matches file
names and paths. Due to the glob based searching, any paths returned from the
method will always represent what's on disk, as opposed to the originating
target path.

This commit updates the File.fnmatch method to instead base the matching on the
canonical directory name (from PDK::Util.canonical_path) which converts
case insensitive targets into their actual on-disk name.

This commit also adds a test for this scenario.


Previously the testing for the parse_targets method was being performed in
multiple places for no added test coverage. Also in order to test this method
required pervasive mocking of the file system which had many side effects. This
commit refactors the parse_targets testing:

  • Any actual File and Dir methods used in parse_targets was refactored to the
    PDK::Util::FileSystem module, in order to make mocking less intrusive. In
    particular the mocking of .file?, .directory? and .glob? made it impossible
    to debug tests because tools like pry use these methods. Without this refactor
    it was impossible to debug tests.

    Note that "readonly" operations like File.join were not refactored as they are
    not mocked in tests and do not actually touch the host operating system,
    unlike glob, and file? etc.

  • The tests for vase_validator were modified to mock the PDK:Util:FileSystem
    module, not the actual File and Dir classes.

  • The puppet_syntax_spec.rb file used to test the .parse_targets method however
    this was unnecssary. The test for the .pattern_ignore property were moved
    from puppet_syntax_spec.rb into base_validator_spec.rb as this is what was
    actually being tested. puppet_syntax_spec.rb now just ensures that the
    pattern_ignore method is set correctly

  • Both the it_accepts_metadata_json_targets and it_accepts_pp_targets helper
    files were testing functionality that was already being tested in the
    base_validator_spec.rb (Namely filtering on pattern). In this case the
    duplicate test is removed and instead we test that the pattern property is as
    we expect.

  • Superfluous mocking of the File class was removed from any tests.

  • Fixed a minor typo in the pattern for the puppet_syntax validator whereby the
    pattern should be '**/*.pp'

  • All of the File and Dir mocking was removed from the name_spec.rb test file
    and instead we just mock the return from the parse_targets method of the base
    validator. The duplicate testing of the parse_targets method we not required
    and actually reflects what the test is testing for.

@coveralls
Copy link

coveralls commented Mar 29, 2019

Coverage Status

Coverage decreased (-0.06%) to 93.011% when pulling 9f33c38 on glennsarti:pdk-1167-case-sensitive-paths into 1ee825e on puppetlabs:master.

@glennsarti glennsarti changed the title (PDK-1167) Validator should honor case sensitive of the file system {WIP}(PDK-1167) Validator should honor case sensitive of the file system Apr 2, 2019
Previously the validator expected that any targets passed into the parse_targets
method were always case sensitive due to the way File.fnmatch matches file
names and paths.  Due to the glob based searching, any paths returned from the
method will always represent what's on disk, as opposed to the originating
target path.

This commit updates the File.fnmatch method to instead base the matching on the
canonical directory name (from PDK::Util.canonical_path) which converts
case insensitive targets into their actual on-disk name.

This commit also adds a test for this scenario.
@glennsarti glennsarti force-pushed the pdk-1167-case-sensitive-paths branch from ec04a0a to af49b2a Compare April 2, 2019 03:06
Previously the testing for the parse_targets method was being performed in
multiple places for no added test coverage. Also in order to test this method
required pervasive mocking of the file system which had many side effects. This
commit refactors the parse_targets testing:

* Any actual File and Dir methods used in parse_targets was refactored to the
  PDK::Util::FileSystem module, in order to make mocking less intrusive.  In
  particular the mocking of .file?, .directory? and .glob? made it impossible
  to debug tests because tools like pry use these methods. Without this refactor
  it was impossible to debug tests.

  Note that "readonly" operations like File.join were not refactored as they are
  not mocked in tests and do not actually touch the host operating system,
  unlike glob, and file? etc.

* The tests for vase_validator were modified to mock the PDK:Util:FileSystem
  module, not the actual File and Dir classes.

* The puppet_syntax_spec.rb file used to test the .parse_targets method however
  this was unnecssary.  The test for the .pattern_ignore property were moved
  from puppet_syntax_spec.rb into base_validator_spec.rb as this is what was
  _actually_ being tested.  puppet_syntax_spec.rb now just ensures that the
  pattern_ignore method is set correctly

* Both the it_accepts_metadata_json_targets and it_accepts_pp_targets helper
  files were testing functionality that was already being tested in the
  base_validator_spec.rb (Namely filtering on pattern). In this case the
  duplicate test is removed and instead we test that the pattern property is as
  we expect.

* Superfluous mocking of the File class was removed from any tests.

* Fixed a minor typo in the pattern for the puppet_syntax validator whereby the
  pattern should be '**/*.pp'

* All of the File and Dir mocking was removed from the name_spec.rb test file
  and instead we just mock the return from the parse_targets method of the base
  validator. The duplicate testing of the parse_targets method we not required
  and actually reflects what the test is testing for.
Previously many of the validate spec tests were created, or moved, so that the
directory structure mimiced that of lib.  However some files were not.  This
commit moves the remaining validation files so that spec mimics lib.
@glennsarti glennsarti force-pushed the pdk-1167-case-sensitive-paths branch from af49b2a to 9f33c38 Compare April 2, 2019 03:15
@glennsarti glennsarti changed the title {WIP}(PDK-1167) Validator should honor case sensitive of the file system (PDK-1167) Validator should honor case sensitive of the file system Apr 2, 2019
@rodjek rodjek merged commit aeec762 into puppetlabs:master Apr 2, 2019
@glennsarti glennsarti deleted the pdk-1167-case-sensitive-paths branch August 19, 2019 08:11
@chelnak chelnak added the bug label Jan 16, 2023
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.

4 participants