Add validation to Registrar. Add RegistrarTest to exercise it. #361
Conversation
validator/src/main/java/com/google/daq/mqtt/registrar/RegistrarTest.java
Outdated
Show resolved
Hide resolved
validator/src/main/java/com/google/daq/mqtt/registrar/LocalDevice.java
Outdated
Show resolved
Hide resolved
| executeWithRegistrar(registrar, argList); | ||
| } | ||
|
|
||
| public static void executeWithRegistrar(Registrar registrar, ArrayList<String> argList) { |
There was a problem hiding this comment.
does this need to be static? Can't it just be a regular class method on Registrar?
There was a problem hiding this comment.
I moved this from main() which was static and was looking to be consistent but make the most minimal change. WDYT?
There was a problem hiding this comment.
New attempt, PTAL
| @@ -705,6 +725,9 @@ private void loadSiteMetadata() { | |||
|
|
|||
| File siteMetadataFile = new File(siteDir, SITE_METADATA_JSON); | |||
| try (InputStream targetStream = new FileInputStream(siteMetadataFile)) { | |||
| // At this time, do not validate the Metadata schema because, by its nature of being | |||
There was a problem hiding this comment.
I'm confused by this comment -- it says "do not validate" and then the code right after is validate()... ?
There was a problem hiding this comment.
Yes but we ignore the return value, the report.
There was a problem hiding this comment.
Still confused -- if it's ignored then why is it calling validate()? Do you mean something like "At this time, only validate the structure of the Metadata schema because it's only a partial overlay, and defer any in-depth validation until after merge?"
validator/src/main/java/com/google/daq/mqtt/registrar/RegistrarTest.java
Outdated
Show resolved
Hide resolved
|
I would make it non-static. I get the minimal change part but in this case
I think it's worth it...
…On Wed, Jun 22, 2022, 2:27 PM John R ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In validator/src/main/java/com/google/daq/mqtt/registrar/Registrar.java
<#361 (comment)>:
> @@ -107,6 +111,10 @@ public class Registrar {
public static void main(String[] args) {
ArrayList<String> argList = new ArrayList<>(List.of(args));
Registrar registrar = new Registrar();
+ executeWithRegistrar(registrar, argList);
+ }
+
+ public static void executeWithRegistrar(Registrar registrar, ArrayList<String> argList) {
I moved this from main() which was static and was looking to be consistent
but make the most minimal change. WDYT?
—
Reply to this email directly, view it on GitHub
<#361 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPDZN7RWNP5GBQQ2QZBDVQOAKXANCNFSM5YYOYBAA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
To me, this would suggest we need to move the argv passing code entirely outside of the class, and pass options into Registrar class, to make a clean boundary. WDYT? |
I'm largely ambivalent about that -- personally I don't see any great value in having a separate class for it, but it doesn't hurt anything. Either way is fine with me...Ideally we'd have more of a shared front-end with common options, etc... but I think that itself would be a bunch of work! |
| } | ||
|
|
||
| private static boolean processArgs(List<String> argList, Registrar registrar) { | ||
| public static void processArgs(List<String> argList, Registrar registrar) { |
There was a problem hiding this comment.
Still not sure why this is static? The general adage with static methods for Java is to avoid them unless they need to be static for some reason. I think this comes down to the issue that you can't mock/override a static method, so it makes future testing/extension a problem sometimes. Not likely in this case, but just on general practice. E.g., I would expect this to be just a regular method on the Registrar class (no need to pass in the explicit parameter). Anyway, that's mainly the Java-style that I've always used, but it's been so long that I'm not 100% sure where it came from. I checked the Java style guide and it doesn't say anything about it. Doesn't really matter -- I also optimize for "least number of words" and if you made it a class method, all references to "registrar." go away reducing the overall footprint of the class -- and there's no world in which you would call this method without a registrar class!
ValidatorTest
Add validation test emptySystemBlock which verifies that removing system{} from metadata produces metadata report
LocalDevice
Add optionally calling validate() on metadata load, and processing the report.
Registrar
Wire controlling validation in LocalDevice through Registrar.
Add option -t to enable validation.
Some refactoring of main for testing purposes.
RegistrarTest
Add test framework for Registrar.
Add a test for successful and failed validation.
Note these tests are not run anywhere -- where should we run them?