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

CB-13471 : added unit test for config file provider #10

Merged
merged 2 commits into from
May 7, 2018

Conversation

audreyso
Copy link
Contributor

No description provided.

@stevengill
Copy link
Contributor

Need to see why this is failing on appveyor

@raphinesse
Copy link
Contributor

raphinesse commented May 4, 2018

@stevengill @audreyso

The condition in this line has to be changed to work on Windows, since file will be res\xml (mind the backslash). Changing the condition to the following fixes it:

file.includes(path.join('res', 'xml'))

However, we still need to extend the test itself. It could be that someone uses the following on a Windows machine:

configFile.resolveConfigFilePath('project_dir', 'android', 'res/xml'))

Then we would fail again. Thus, we have to normalize the given path right at the start of configFile.resolveConfigFilePath:

file = path.normalize(file);

These two changes taken together will make the following test (and all others) pass on Windows:

it('resolveConfigFilePath should return file path', function () {
    [path.join('res', 'xml'), 'res/xml'].forEach(function (file) {
        var configPath = path.join('project_dir', 'app', 'src', 'main', file, 'xml');
        expect(configFile.resolveConfigFilePath('project_dir', 'android', file)).toBe(configPath);
    });
});

@dpogue
Copy link
Member

dpogue commented May 5, 2018

Thanks @raphinesse! I've added a commit (attributed to you) with those changes, and fingers crossed for passing CI

@codecov-io
Copy link

codecov-io commented May 5, 2018

Codecov Report

Merging #10 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   85.22%   85.23%   +<.01%     
==========================================
  Files          19       19              
  Lines        1767     1768       +1     
  Branches      376      376              
==========================================
+ Hits         1506     1507       +1     
  Misses        261      261
Impacted Files Coverage Δ
src/ConfigChanges/ConfigFile.js 89.13% <100%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8cca53...296a003. Read the comment docs.

@raphinesse
Copy link
Contributor

Thanks for adding the commit and responding so quickly, @dpogue!

@dpogue dpogue merged commit 5bc0764 into apache:master May 7, 2018
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

5 participants