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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added test Framework Utility Functions For the json Schema #1078

Open
wants to merge 22 commits into
base: master
from

Conversation

@sladyn98
Copy link
Contributor

sladyn98 commented Sep 27, 2019

This PR deals with the addition of a test framework to test the new JSON Schema.
Closes : #997

Your checklist for this pull request

馃毃 Please review the guidelines for contributing to this repository.

  • [x ] Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • [ x] Ensure that the pull request title represents the desired changelog entry
  • [x ] Please describe what you did
  • [ x] Link to relevant issues in GitHub or in Jenkins JIRA
  • Link to relevant pull requests, esp. upstream and downstream changes
  • [x ] Did you provide a test-case? That demonstrates feature works or fixes the issue.
@sladyn98 sladyn98 requested review from casz, oleg-nenashev and timja and removed request for casz Sep 27, 2019
Copy link
Member

timja left a comment

Some initial thoughts

@sladyn98 sladyn98 requested a review from timja Sep 28, 2019
*Validates a given jsonObject against the schema generated for the current live jenkins instance
* * Example Usage:
* * * <pre>{@code
* * * assertTrue(validateSchema(jsonSubject));}

This comment has been minimized.

Copy link
@timja

timja Sep 28, 2019

Member

I think a nicer usage could be something like:

Suggested change
* * * assertTrue(validateSchema(jsonSubject));}
* * * assertThat(yaml, isValidJsonSchema()));}
@timja timja requested a review from casz Sep 28, 2019
@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Sep 28, 2019

thoughts on writing a custom matcher?
https://www.baeldung.com/hamcrest-custom-matchers

Advantages of doing so ? The current validation works just as well right, I mean since we have only objects and strings and numbers to an extent

@timja

This comment has been minimized.

Copy link
Member

timja commented Sep 28, 2019

thoughts on writing a custom matcher?
baeldung.com/hamcrest-custom-matchers

Advantages of doing so ? The current validation works just as well right, I mean since we have only objects and strings and numbers to an extent

It's not about the validation working it's about making the test easier to read and in a more natural way,

instead of
assert true, valid schema
assert that schema is valid

may or may not be worth it...

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Sep 28, 2019

thoughts on writing a custom matcher?
baeldung.com/hamcrest-custom-matchers

Advantages of doing so ? The current validation works just as well right, I mean since we have only objects and strings and numbers to an extent

It's not about the validation working it's about making the test easier to read and in a more natural way,

instead of
assert true, valid schema
assert that schema is valid

may or may not be worth it...

If you want we could add it as a goal, but we would address it at the end ?

Copy link
Member

oleg-nenashev left a comment

A lot of comments, but it definitely moves in a right direction. Thanks!

* * <pre>{@code
* * Schema jsonSchema = returnSchema();}
* * </pre>
* @return

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 28, 2019

Member

Please avoid using empty tags, it just leads to Javadoc warnings

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 28, 2019

Author Contributor

Got it :)

ve.printStackTrace();
yamlStringContents = Util.toStringFromYamlFile(this, yamlFile);
} catch (Exception e) {
e.printStackTrace();

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 28, 2019

Member

Please avoid doing so in modern test tools or the production code. System logging is your friend

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 28, 2019

Author Contributor

Yeah added the System Logging

@sladyn98 sladyn98 requested review from oleg-nenashev and timja Sep 29, 2019
@sladyn98 sladyn98 self-assigned this Sep 29, 2019
@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Sep 29, 2019

@timja I will open a ticket for the custom Matchers so that we can deal with that in a different PR, anything else needed in this PR ?

ve.printStackTrace();
yamlStringContents = Util.toStringFromYamlFile(this, yamlFile);
} catch (Exception e) {
LOGGER.warning(stringToYAMLConversionFailure);

This comment has been minimized.

Copy link
@timja

timja Sep 29, 2019

Member

shouldn't this just throw an exception? or is there a reason you're logging and continuing?

This comment has been minimized.

Copy link
@timja

timja Sep 29, 2019

Member

I would throw some form of runtime exception

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Sep 29, 2019

Author Contributor

Yeah I will throw an exception

@timja
timja approved these changes Oct 1, 2019
Add a test for the YAML file
```

This comment has been minimized.

Copy link
@timja

timja Oct 7, 2019

Member
Suggested change
```
```java
You can test if your YAML file validates against the schema.
### Step 1

This comment has been minimized.

Copy link
@timja

timja Oct 7, 2019

Member
Suggested change
### Step 1
#### Step 1
```
### Step 2

This comment has been minimized.

Copy link
@timja

timja Oct 7, 2019

Member
Suggested change
### Step 2
#### Step 2
@sladyn98 sladyn98 requested a review from timja Oct 7, 2019
@@ -99,6 +99,11 @@
<version>0.50.40</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.vaadin.external.google</groupId>
<artifactId>android-json</artifactId>

This comment has been minimized.

Copy link
@timja

timja Oct 7, 2019

Member

where has this came from?

This comment has been minimized.

Copy link
@sladyn98

sladyn98 Oct 7, 2019

Author Contributor

Strange sounds like it imported a different json

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Oct 7, 2019

@timja codacy still crying 馃槩

@sladyn98 sladyn98 requested a review from timja Oct 7, 2019
docs/PLUGINS.md Outdated Show resolved Hide resolved
docs/PLUGINS.md Outdated Show resolved Hide resolved
sladyn98 and others added 2 commits Oct 8, 2019
This header was causing codacy warnings

Co-Authored-By: Tim Jacomb <t.jacomb@kainos.com>
Co-Authored-By: Tim Jacomb <t.jacomb@kainos.com>
@timja
timja approved these changes Oct 8, 2019
Add a test for the YAML file
```java
@Test

This comment has been minimized.

Copy link
@timja

timja Oct 8, 2019

Member

formatting is off:
image

@sladyn98 sladyn98 requested a review from timja Oct 8, 2019
@timja
timja approved these changes Oct 8, 2019
Copy link
Member

oleg-nenashev left a comment

Exceptions must not be used in APIs, It is basically "I will throw whatever I want", and it breaks compatibility. Please use an existing checked exception type... or introduce a new one like https://github.com/jenkinsci/configuration-as-code-plugin/blob/master/plugin/src/main/java/io/jenkins/plugins/casc/ConfiguratorException.java

@@ -16,7 +16,7 @@
.put("description", "Jenkins Configuration as Code")
.put("type", "object");

public static JSONObject generateSchema() {
public static JSONObject generateSchema() throws Exception {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 11, 2019

Member

Breaks compatibility for API consumers. Not a big deal in this case, but you might want to mark the plugin as restricted

This comment has been minimized.

Copy link
@timja

timja Oct 18, 2019

Member

Is the throws exception actually needed? I can't see a change that requires this?

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Oct 12, 2019

@oleg-nenashev So I should add a @restricted at the top of the function ? Or define a new Exception?

@sladyn98

This comment has been minimized.

Copy link
Contributor Author

sladyn98 commented Nov 7, 2019

@timja I guess removing this should work.

sladyn98 added 2 commits Nov 7, 2019
@sladyn98 sladyn98 requested review from timja and oleg-nenashev Nov 7, 2019
@timja
timja approved these changes Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can鈥檛 perform that action at this time.