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

configurable NA and validation #29

Closed
wants to merge 4 commits into from

Conversation

tkruse
Copy link
Contributor

@tkruse tkruse commented Jun 29, 2018

The PR makes it possible for library users of TCases to override the "NA" value for "not applicable, and the rules for validation of string values that are part of an inputDef or testDef.

I can imagine several further patches to make Tcases more configurable as a library. The key idea is that customizing users are responsible themselves if anything breaks, but they can make many customizations without having to fork tcases, and thus also patch bugs easily instead of depending on you to make a new release.

I am not particularly proud of using the Globals class like that, but short of adding a DI framework like Guice or Dagger, I am not sure how the codebase could be made configurable with justifiable effort.

@tkruse
Copy link
Contributor Author

tkruse commented Jun 29, 2018

This does not solve the general issue that NA should have a special markup in XML, and not merely value="NA"

@tkruse
Copy link
Contributor Author

tkruse commented Jun 29, 2018

With the PR applied, I can run tests with custom symbols like this

        Globals.setValidator(new DefinitionsValidator() {
            @Override
            public void assertValueName(String name) {
                if (name == null) {
                    throw new IllegalStateException("name must not be null");
                }
            }

            @Override
            public void assertVarBindingValue(String name) {
                if (name == null) {
                    throw new IllegalStateException("name must not be null");
                }
            }
        });

        Globals.setXmlWriterFactory(new XmlWriterFactory() {
            class PatchedXmlWriter extends XmlWriter {
                public PatchedXmlWriter(OutputStream output) {
                    super(output);
                }
                public PatchedXmlWriter(Writer writer) {
                    super(writer);
                }

                @Override
                public void writeAttribute(String name, String value) {
                    print( " ");
                    print( name);
                    print( "=\"");
                    print( StringEscapeUtils.escapeXml10(value));
                    print( "\"");
                }
            }

            @Override
            public XmlWriter forStream(OutputStream output) {
                return new PatchedXmlWriter(output);
            }

            @Override
            public XmlWriter forWriter(Writer writer) {
                return new PatchedXmlWriter(writer);
            }
        });

@kerrykimbrough
Copy link
Contributor

I'm not happy with the Globals class, either. I realize this approach seems more tractable. But it doesn't really address the problem of Tcases exposing parameters that logically should be definable by the system input model. Any solution ought to allow a single program to create and manage two SystemInputDef instances with different values for the NA string.

@kerrykimbrough
Copy link
Contributor

I'm especially dubious of all the other Globals methods unrelated to the original NA issue. Those methods change things that I don't think really deserve to be configurable. Perhaps they originate from concerns addressed by PR #27, in which case they are now superfluous.

@tkruse
Copy link
Contributor Author

tkruse commented Jun 30, 2018

we can put this one on hold. if #27 can be merged this one is less urgent for me.

But generally speaking I regard the value of Tcases as a library for other projects mostly in the way it does combinatorial generation of cases. All the other restrictions and conventions Tcases enforces get in the way of other use-cases of the library. E.g. I wish also for more liberty in defining property values. The naming restrictions might make sense for the purpose of Tcases as an application to manage xml files, but they are not functionally required by the combinatorial algorithm, so they are bound to get in the way. Even in XML, it seems most could be much more liberal, so the restriction to the identifier pattern from DefUtils seems like an arbitrary subjective choice.
Ideally the algorithm would be accessible with a minimum amount of restrictions, and all restrictions and validations would happen outside of where serialization and deserialization to/from XML happens.

Other than using a DI approach (like Guice or Dagger), something similar could also be achieved by using the Abstract Factory Pattern in all places where Tcases currently directly calls new Xyz(), similar to what this PR does for XmlWriter. The factory instances could be passed on through constructor chains, but it would look more messy than now in a way (which is why DI frameworks exist).

@kerrykimbrough
Copy link
Contributor

To address issue #16, different solutions may be better.

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