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

Added tests and refactored code (Part 1) #3

Merged
merged 6 commits into from
Jan 18, 2023
Merged

Added tests and refactored code (Part 1) #3

merged 6 commits into from
Jan 18, 2023

Conversation

ijlee2
Copy link
Owner

@ijlee2 ijlee2 commented Jan 18, 2023

Description

Currently, we assert at an "acceptance" level that ember-codemod-v1-to-v2 can run all steps without encountering an error. However, there are no "integration" tests that explain the output of each step. Such tests would help contributors (including myself in the future) understand the codebase.

I'll create a series of pull requests to incrementally add tests for each step. In this pull request, tests for the following steps have been added:

  • augmentOptions
  • analyzeAddon
  • useRelativePaths

@ijlee2 ijlee2 added the enhance: documentation Issue asks for better documentation (e.g. README, code, tests) label Jan 18, 2023
@ijlee2 ijlee2 marked this pull request as ready for review January 18, 2023 09:17
Comment on lines +71 to +78
throw new SyntaxError(
'ERROR: In package.json, the package name is missing.'
);
}

if (!addonPackage.version) {
throw new SyntaxError(
'ERROR: In package.json, the package version is missing.'
Copy link
Owner Author

Choose a reason for hiding this comment

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

According to npm, we can expect that a valid package.json will always have the name and version fields.

https://docs.npmjs.com/creating-a-package-json-file#required-name-and-version-fields

Comment on lines -71 to -73
packagesToDelete.forEach((packageName) => {
devDependencies.delete(packageName);
});
Copy link
Owner Author

Choose a reason for hiding this comment

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

Unused code. To be precise, a no-op because devDependencies becomes an empty map after line 69.

Comment on lines +64 to +66
packageJson['keywords'] = (packageJson['keywords'] ?? []).filter(
(keyword) => keyword !== 'ember-addon'
);
Copy link
Owner Author

Choose a reason for hiding this comment

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

The package.json in an Ember addon should have the keywords field, which is an array that includes 'ember-addon'. Just in case, I used the nullish coalescing operator to prevent a runtime error.

@@ -10,8 +10,8 @@ test('migration | ember-addon | steps | augment-options > error handling (corrup
};

const inputProject = {
'package.json': '{\n name:}',
'yarn.lock': 'some code for yarn.lock',
'package.json': '{\n "name": }',
Copy link
Owner Author

Choose a reason for hiding this comment

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

I meant to test what would happen when the value is missing.

Instead, I had incorrectly tested the case of a JSON key having an incorrect format (because the double quotations are missing).

` Features,`,
` IndexSignatureParameter,`,
` QueryResults,`,
`} from '../modifiers/container-query';`,
Copy link
Owner Author

Choose a reason for hiding this comment

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

Here, we're asserting that useRelativePaths ...

  • Switches Ember's "magic" import path, ember-container-query/modifiers/container-query, to JavaScript's relative path, ../modifiers/container-query
  • Does not alter the rest of the file

Comment on lines +52 to +53
`import type { Image } from '../../../../data/concert';`,
`import { findBestFittingImage } from '../../../../utils/components/widgets/widget-3';`,
Copy link
Owner Author

@ijlee2 ijlee2 Jan 18, 2023

Choose a reason for hiding this comment

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

Here, we're asserting that useRelativePaths ...

  • Finds and replaces multiple "magic" paths in a given file


output: [
`import Application from '@ember/application';`,
`import config from './config/environment';`,
Copy link
Owner Author

@ijlee2 ijlee2 Jan 18, 2023

Choose a reason for hiding this comment

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

Here, we're asserting that useRelativePaths ...

  • Normalizes the relative path, i.e. if the referenced file is in the same directory as the file that is about to be modified, then the path should be prefixed with ./.

    In other words, we should not be seeing the line import config from 'config/environment';, because this implies there is a package named config.

@ijlee2 ijlee2 merged commit eee5a79 into main Jan 18, 2023
@ijlee2 ijlee2 deleted the refactor-code branch January 18, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhance: documentation Issue asks for better documentation (e.g. README, code, tests)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant