breautek commented on code in PR #212:
URL: https://github.com/apache/cordova-common/pull/212#discussion_r1729388351
##########
spec/ConfigChanges/ConfigFile.spec.js:
##########
@@ -78,14 +78,19 @@ describe('ConfigFile tests', function () {
expect(ConfigFile.resolveConfigFilePath('project_dir',
'android', 'strings.xml')).toBe(stringsPath);
});
+ it('should return android values xml file path', function () {
+ const resPath = path.join(projectDir, 'res', 'values',
'colors.xml');
+ expect(ConfigFile.resolveConfigFilePath('project_dir',
'android', path.join('res', 'values', 'colors.xml'))).toBe(resPath);
+ });
+
it('should return ios config.xml file path', function () {
- spyOn(ConfigFile,
'getIOSProjectname').and.returnValue('iospath');
+ ConfigFile.__set__('getIOSProjectname', () => 'iospath');
Review Comment:
I think we should avoid using `rewire` when possible and stick with the
jasmine API for mocking.
Rewire arguably promotes [bad
practices](https://medium.com/decathlondigital/should-i-test-private-methods-direclty-c48f4fa7bb4d)
and sticking with Jasmine will make the tests more simple.
I'd like to see `rewire` be completely removed eventually but of course
there is a lot of legacy code, and perhaps code that isn't easily unit testable
without it (which is a problem with the code in question)
##########
src/ConfigChanges/ConfigFile.js:
##########
@@ -248,16 +267,19 @@ function getIOSProjectname (project_dir) {
throw new Error(`${msg} in ${project_dir}`);
}
- return path.basename(matches[0], '.xcodeproj');
+ const projectName = path.basename(matches[0], '.xcodeproj');
+ xcodeprojMap.set(project_dir, projectName);
+ return projectName;
}
// determine if a plist file is binary
// i.e. they start with the magic header "bplist"
+// TODO: Remove in next major
function isBinaryPlist (filename) {
return readChunk.sync(filename, 0, 6).toString() === 'bplist';
}
module.exports = ConfigFile;
-module.exports.isBinaryPlist = isBinaryPlist;
+module.exports.isBinaryPlist = util.deprecate(isBinaryPlist, 'isBinaryPlist is
deprecated');
module.exports.getIOSProjectname = getIOSProjectname;
Review Comment:
On the jasmine vs rewire thing.. this might have been a cause of confusion.
The jasmine way worked because it is spying on `ConfigFile`, and the class
definition lacks `getIOSProjectname`... but it gets added on this line (because
`module.exports === ConfigFile`, so adding properties on `module.exports` is
adding properties on `ConfigFile`).
Perhaps out of scope of this PR but it would make more sense if
`getIOSProjectname` becomes an actual static method, especially since we
already use [ES6
classes](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/static#browser_compatibility):
```javascript
class ConfigFile {
...
static getIOSProjectname (project_dir) {
...
}
}
```
Then we won't need that `module.exports.getIOSProjectname = ...` line
##########
spec/ConfigChanges/ConfigFile.spec.js:
##########
@@ -116,6 +121,21 @@ describe('ConfigFile tests', function () {
expect(ConfigFile.resolveConfigFilePath('', 'ios',
'*-Info.plist')).toBe(expectedPlistPath);
});
+
+ it('should return Info.plist file', function () {
+ const projName = 'XXX';
+ const expectedPlistPath = path.join(projName, 'Info.plist');
+
+ ConfigFile.__set__('getIOSProjectname', () => projName);
Review Comment:
The jasmine equivalent should be:
```suggestion
spyOn(ConfigFile,
'getIOSProjectname').and.returnValue(projName);
```
##########
spec/ConfigChanges/ConfigFile.spec.js:
##########
@@ -78,14 +78,19 @@ describe('ConfigFile tests', function () {
expect(ConfigFile.resolveConfigFilePath('project_dir',
'android', 'strings.xml')).toBe(stringsPath);
});
+ it('should return android values xml file path', function () {
+ const resPath = path.join(projectDir, 'res', 'values',
'colors.xml');
+ expect(ConfigFile.resolveConfigFilePath('project_dir',
'android', path.join('res', 'values', 'colors.xml'))).toBe(resPath);
+ });
+
it('should return ios config.xml file path', function () {
- spyOn(ConfigFile,
'getIOSProjectname').and.returnValue('iospath');
+ ConfigFile.__set__('getIOSProjectname', () => 'iospath');
const configPath = path.join('project_dir', 'iospath',
'config.xml');
expect(ConfigFile.resolveConfigFilePath('project_dir', 'ios',
'config.xml')).toBe(configPath);
});
it('should return osx config.xml file path', function () {
- spyOn(ConfigFile,
'getIOSProjectname').and.returnValue('osxpath');
+ ConfigFile.__set__('getIOSProjectname', () => 'osxpath');
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
