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

Fix bug4793 in assembly #433

Merged
merged 19 commits into from
Jun 30, 2019
Merged

Fix bug4793 in assembly #433

merged 19 commits into from
Jun 30, 2019

Conversation

allaVolkov
Copy link
Contributor

Fix bug4793 in assembly

Checklist

  • Code compiles correctly
  • Relevant tests were added (unit / contract / integration)
  • Relevant logs were added
  • Formating and linting run locally successfully
  • All tests passing
  • UA review
  • Design is documented
  • Extended the README / documentation, if necessary
  • Open source is approved

@allaVolkov allaVolkov requested review from tal-sapan and ShimiT June 25, 2019 18:37
Copy link
Contributor

@tal-sapan tal-sapan left a comment

Choose a reason for hiding this comment

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

Test files were not reviewed yet

internal/archive/fsops.go Outdated Show resolved Hide resolved
internal/artifacts/module_arch.go Outdated Show resolved Hide resolved
internal/artifacts/module_arch.go Outdated Show resolved Hide resolved
internal/buildops/build_params.go Outdated Show resolved Hide resolved
internal/buildops/build_params.go Outdated Show resolved Hide resolved
internal/artifacts/module_arch.go Outdated Show resolved Hide resolved
internal/artifacts/module_arch.go Outdated Show resolved Hide resolved
internal/artifacts/manifest.go Outdated Show resolved Hide resolved
internal/archive/fsops.go Outdated Show resolved Hide resolved
internal/artifacts/manifest.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tal-sapan tal-sapan left a comment

Choose a reason for hiding this comment

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

Still without the tests

internal/archive/fsops.go Outdated Show resolved Hide resolved
internal/archive/fsops.go Outdated Show resolved Hide resolved
internal/artifacts/manifest.go Show resolved Hide resolved
internal/artifacts/module_arch.go Outdated Show resolved Hide resolved
internal/artifacts/module_arch.go Outdated Show resolved Hide resolved
internal/artifacts/module_arch.go Outdated Show resolved Hide resolved
internal/buildops/build_params.go Outdated Show resolved Hide resolved
internal/buildops/build_params.go Outdated Show resolved Hide resolved
internal/artifacts/artifacts_suite_test.go Outdated Show resolved Hide resolved
internal/artifacts/module_arch.go Outdated Show resolved Hide resolved
internal/buildops/build_params.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tal-sapan tal-sapan left a comment

Choose a reason for hiding this comment

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

There's still an unresolved question in manifest.go

@@ -37,22 +37,25 @@ var _ = Describe("Commands", func() {
})

It("Generate Meta", func() {
os.MkdirAll(getTestPath("result", ".mtahtml5_mta_build_tmp", "testapp"), os.ModePerm)
dir.CreateDirIfNotExist(getTestPath("result", ".mtahtml5_mta_build_tmp", "testapp"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this folder have to be created? Shouldn't it exist only in the project's sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, when creating manifest we check target artifacts in temp folder

Copy link
Contributor

Choose a reason for hiding this comment

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

The module's temp folder is under the module's name, not its path, isn't it?

cmd/gen_test.go Outdated Show resolved Hide resolved
cmd/gen_test.go Outdated Show resolved Hide resolved
integration/testdata/mta_demo/node/package-lock.json Outdated Show resolved Hide resolved
os.MkdirAll(getTestPath("result", ".mtahtml5_mta_build_tmp", "testapp"), os.ModePerm)
os.MkdirAll(getTestPath("result", ".mtahtml5_mta_build_tmp", "ui5app2"), os.ModePerm)
os.MkdirAll(getTestPath("result", ".mtahtml5_mta_build_tmp", "META-INF"), os.ModePerm)
createMtahtml5TmpFolder()
file, _ := os.Create(getTestPath("result", ".mtahtml5_mta_build_tmp", "xs-security.json"))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this (it's done inside createMtahtml5TmpFolder)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still here...

internal/artifacts/meta_test.go Outdated Show resolved Hide resolved
internal/artifacts/meta_test.go Show resolved Hide resolved
internal/artifacts/meta_test.go Outdated Show resolved Hide resolved
internal/buildops/build_params_test.go Outdated Show resolved Hide resolved
@allaVolkov

This comment has been minimized.

@tal-sapan
Copy link
Contributor

The coverage fell because of these missing tests:

  • GetModuleTargetArtifactPath is not tested (also getArtifactInfo, but it's called from GetModuleTargetArtifactPath). I'm not sure why it isn't reported since packModule is tested and it calls it.
  • Non-string build-result in GetModuleSourceArtifactPath
  • getMtad with bad platform config - there's a test for this, but the error that's returned is probably due to another reason now (I commented on it in the code review)
  • Call to Archive when the target folder of the zip is a file
  • Unknown contentType in getModuleEntries (when the file exists)
  • Call to copyModuleArchiveToResultDir when the target folder of the archive is a file
  • CopyFile fails in copyModuleArchiveToResultDir (not sure when that would happen)

internal/archive/fsops_test.go Outdated Show resolved Hide resolved
internal/artifacts/meta_test.go Outdated Show resolved Hide resolved
internal/buildops/build_params.go Show resolved Hide resolved
internal/buildops/build_params_test.go Show resolved Hide resolved
internal/artifacts/manifest_test.go Outdated Show resolved Hide resolved
internal/artifacts/manifest_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@tal-sapan tal-sapan left a comment

Choose a reason for hiding this comment

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

All the unresolved comments are still relevant, but if it's urgent to release they can be fixed on another PR since they are on the tests and aren't critical.

@allaVolkov allaVolkov merged commit 819692f into master Jun 30, 2019
@tal-sapan tal-sapan mentioned this pull request Jul 2, 2019
9 tasks
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.

2 participants