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

Added additional verification for GenerateChangelogTest #486

Merged
merged 15 commits into from
Feb 3, 2023

Conversation

yodzhubeiskyi
Copy link
Contributor

No description provided.

@yodzhubeiskyi yodzhubeiskyi marked this pull request as ready for review February 2, 2023 08:50
@PavloTytarchuk PavloTytarchuk self-requested a review February 2, 2023 11:48
@@ -24,6 +24,7 @@ class GenerateChangelogTest extends Specification {
@Shared
UIService uiService = Scope.getCurrentScope().getUI()
String testResourcesPath = System.getProperty("user.dir") + "/src/test/resources/"
String generatedResourcesPath = "/src/test/resources/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz change to something like
String resourcesDirPath = "/src/test/resources/"
String resourcesDirFullPath = System.getProperty("user.dir") + resourcesDirPath

Copy link
Contributor

@KushnirykOleh KushnirykOleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expectedChangeLog/temp folder is confusing

Copy link
Contributor

@KushnirykOleh KushnirykOleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's either have 4 properties
String xmlChangelogPath
String sqlChangelogPath
String yamlChangelogPath
String jsonChangelogPath
or only base path
as

map.put("expectedYmlChangelog", testInput.xmlChangelogPath.replace(".xml", ".yml"))
map.put("expectedJsonChangelog", testInput.xmlChangelogPath.replace(".xml", ".json"))

is confusing

Copy link
Contributor

@KushnirykOleh KushnirykOleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to move

        argsMap.put("excludeObjects", "(?i)posts, (?i)authors")

up where other arguments are put into argsMap, it looks excessive in place where we fill map

@@ -28,7 +28,7 @@ class GenerateChangelogTestHelper {
for (DatabaseUnderTest databaseUnderTest : databaseConnectionUtil
.initializeDatabasesConnection(TestConfig.instance.getFilteredDatabasesUnderTest())) {
for (def changeLogEntry : FileUtils.resolveInputFilePaths(databaseUnderTest, baseChangelogPath +
"expectedChangeLog", "sql").entrySet()) {
"expectedChangeLog", "xml").entrySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use TestConfig.instance.inputFormat here? Or maybe we can change to build testInput in other way as we don't really pick up only files with this extension

@yodzhubeiskyi yodzhubeiskyi merged commit 0d5cdde into main Feb 3, 2023
@yodzhubeiskyi yodzhubeiskyi deleted the DAT-12909 branch February 3, 2023 11:55
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