Percona-Lab / pg_tde

MIT License
107 stars 19 forks source link

#243: Rename pgtde_is_encrypted() to pg_tde_is_encrypted() #242

Closed jeffreydwalter closed 2 months ago

jeffreydwalter commented 2 months ago

See: https://github.com/Percona-Lab/pg_tde/issues/243

jeffreydwalter commented 2 months ago

Happy to help!

dAdAbird commented 2 months ago

It needs a couple of more changes in tests' expected files. See the regression.diff here: https://github.com/Percona-Lab/pg_tde/actions/runs/10025861865/artifacts/1725344792

jeffreydwalter commented 2 months ago

It needs a couple of more changes in tests' expected files. See the regression.diff here: https://github.com/Percona-Lab/pg_tde/actions/runs/10025861865/artifacts/1725344792

@dAdAbird can you please tell me what changes need to be made? It's not really obvious to me, from the diff, what the problem is. It looks like it might be a GitHub action cache issue.

dutow commented 2 months ago

@jeffreydwalter The dashed lines need one more dash because now the function name is one character longer. It's more visible if you add a space manually to the diff - without that, it's difficult to see because the diff also uses the - as a marker:

 SELECT pg_tde_is_encrypted('test_norm');
  pg_tde_is_encrypted 
-  --------------------
+  ---------------------

You can also execute the tests locally and use the test runner to generate the expected files, that prevents issues like this.

jeffreydwalter commented 2 months ago

@jeffreydwalter The dashed lines need one more dash because now the function name is one character longer. It's more visible if you add a space manually to the diff - without that, it's difficult to see because the diff also uses the - as a marker:

 SELECT pg_tde_is_encrypted('test_norm');
  pg_tde_is_encrypted 
-  --------------------
+  ---------------------

You can also execute the tests locally and use the test runner to generate the expected files, that prevents issues like this.

Ah, I opened the diff in a text editor, so it wasn't obvious because the - blended into the line. 😆 image

Anyway, I think I get why you guys do testing that way, but surely there's got to be a better way... Thanks for your help!

dAdAbird commented 2 months ago

Yeah, those alignment things might be quite annoying. But renaming or similar changes concerning existing tests shouldn't happen much... Anyway, thanks for your contribution