test: Fix bug in TestAddTestPlugin test helper

[fairclothjm]

This PR fixes a bug in TestAddTestPlugin that appears to only manifest on Linux systems. This bug resulted in the test failing on Mac systems but passing on Linux systems because we failed to properly get the plugin version.

Additionally, we update the test to use the plugin version when performing the Read on the plugin catalog since this should now be properly set.

We write the plugin binary file in TestAddTestPlugin and then call file.Close() in a defer statement. However, we need to read the file's SHA256 sum immediately after writing the file so we call file.Sync(). On Linux this was not working as expected because we seem to be reading the sha sum and running the plugin too fast. This caused the call to get the plugin version to fail and to be incorrectly written to storage as un-versioned.

[fairclothjm]

I think its ok to leave the defer out.Close() call even though it will error since we are ignoring it?

[maxcoulombe]

This caused the call to get the plugin version to fail and to be incorrectly written to storage as un-versioned.

Is this intended? Should the plugin addition/registration operation fail entirely if we expect a version but fail to retrieve it? Maybe that's lenient for backward compatibility or something but I'm wondering.

[swenson]

LGTM

And yes, exactly, I think we can leave in defer out.Close() since we ignore the error (and it's good to keep it in just in case we hit one of the t.Fatal lines).

[swenson]

This caused the call to get the plugin version to fail and to be incorrectly written to storage as un-versioned.

Is this intended? Should the plugin addition/registration operation fail entirely if we expect a version but fail to retrieve it? Maybe that's lenient for backward compatibility or something but I'm wondering.

This is intended. We don't know if the version is meant to be versioned until we run it and try to query the gRPC endpoint for getting the version. If it fails, we assume the plugin is unversioned.

We could possibly tighten the error handling so that we only register it if we get the right gRPC error, and fail to register the plugin on other errors.

[fairclothjm]

@maxcoulombe

Is this intended? Should the plugin addition/registration operation fail entirely if we expect a version but fail to retrieve it? Maybe that's lenient for backward compatibility or something but I'm wondering.

Yes, I believe it is for backwards compat but also plugins can self-report their version. We make a best effort to get the plugin version on registration but we don't fail the request if we can't.

[fairclothjm]

backporting to 1.12.x and 1.13.x because this problem exists since plugin versioning was introduced.