nodejs: check why we're deleting smoke tests

[jskeet]

googleapis/google-cloud-node#8552 deletes packages/google-cloud-iot/smoke-test/device_manager_smoke_test.js... I'm not sure why. Perhaps combine-libraries?

[jskeet]

Likewise googleapis/google-cloud-node#8547 deletes packages/google-cloud-billing-budgets/system-test/system.js

[jskeet]

See also googleapis/google-cloud-node#8545

[jskeet]

See also googleapis/google-cloud-node#8576

[jskeet]

Related: googleapis/google-cloud-node#8580 deletes utilities in the test directory. (If this is fully generated, do we need those?)

[jskeet]

See also: googleapis/google-cloud-node#8621

[JoeWang1127]

For the short term, add smoke test to keep list.

Investigate why smoke test is removed during combine library phase in librarian. Remove smoke test from keep once fixing it in librarian.

[JoeWang1127]

The reason of smoke tests are removed is that all directories and files are cleaned during the execution of gapic-node-processing combine-library ...:
https://github.com/googleapis/google-cloud-node/blob/main/core/packages/gapic-node-processing/src/combine-libraries.ts#L269

Since smoke tests are not generated, we need to add them to the keep list in order to preserve them.

Another approach is to keep files in smoke-test by default. I don't think this is a good idea since only four packages have this directory, we can just expend the keep list if we want to save them.

./packages/google-cloud-dataproc/smoke-test
./packages/google-cloud-bigquery-datatransfer/smoke-test
./packages/google-cloud-vision/smoke-test
./packages/google-cloud-iot/smoke-test

@jskeet what do you think?

[jskeet]

Right, so it looks like that bit of combine-libraries is effectively another version of librarian clean. I personally don't like the redundancy here.

I think it would be worth figuring out which aspects of combine-libraries we want to keep, and then we can either change gapic-node-processing to make it do only the bits we want, or rewrite it in Go.

#6262 takes an alternative approach: keep combine-libraries, but give it a new directory as the destination. So it has nothing to delete to start with - and we check that we're deleting everything we should be deleting in librarian clean because os.CopyFS will fail if we try to overwrite a file. (In other words, everything that combine-libraries says should be in the library must be cleaned before we copy the result into the destination.) That removes the redundancy/conflicts of "which files should be deleted" being decided in two different places.

The disadvantage of that approach is that it doesn't allow combine-libraries to use any other files (e.g. google-cloud-translate/src/v2/index.ts)... but maybe that's okay.

[JoeWang1127]

I'd like us to rewrite combine-libraries in librarian, b/c it would be weird to define a separate post processing logic out of librarian.

I can close this issue and create a new one for migrating combine-libraries, what do you think?

[jskeet]

I think closing this issue is fine, as it was mostly a matter of "work out what code is doing it" which you've done.

A new issue for "what does combine-libraries do, what does it do that we don't want to do, and how do we reimplement it" would also be good - but I wouldn't assume a timeline for doing that vs (maybe) merging #6262 as an interim approach.

[JoeWang1127]

but I wouldn't assume a timeline for doing that

Yes, we don't need to implement it now but it would be one task for the post migration. FYI, we also have an [issue](but I wouldn't assume a timeline for doing that vs (maybe) merging #6262 as an interim approach.) to rewrite generate-reame in the backlog.

(maybe) merging #6262 as an interim approach.

SGTM, could you update the pull request and I'll review it.

[jskeet]

(maybe) merging #6262 as an interim approach.

SGTM, could you update the pull request and I'll review it.

We need to investigate regenerating package.json before we merge it, and I don't really want to try to do that until we're done with the first tranche of migration - but it would probably still be worth you reviewing now in terms of the change to the process, so that you understand the aims.

[JoeWang1127]

Created #6428 for rewriting combine-libraries.