jamesfredley commented on code in PR #15654:
URL: https://github.com/apache/grails-core/pull/15654#discussion_r3314876200
##########
grails-data-mongodb/core/src/test/groovy/org/apache/grails/data/mongo/core/GrailsDataMongoTckManager.groovy:
##########
@@ -18,40 +18,37 @@
*/
package org.apache.grails.data.mongo.core
-import groovy.util.logging.Slf4j
-
import com.mongodb.BasicDBObject
import com.mongodb.client.MongoClient
-import org.bson.Document
-import org.slf4j.LoggerFactory
-import org.testcontainers.containers.MongoDBContainer
-import org.testcontainers.containers.output.Slf4jLogConsumer
-
-import org.springframework.context.support.GenericApplicationContext
-import org.springframework.context.support.StaticMessageSource
-import org.springframework.validation.Validator
-
import grails.core.DefaultGrailsApplication
import grails.core.GrailsApplication
import grails.gorm.validation.PersistentEntityValidator
+import groovy.util.logging.Slf4j
import org.apache.grails.data.testing.tck.base.GrailsDataTckManager
import org.apache.grails.testing.mongo.AbstractMongoGrailsExtension
+import org.bson.Document
import org.grails.datastore.bson.query.BsonQuery
import org.grails.datastore.gorm.GormEnhancer
import org.grails.datastore.gorm.mongo.Birthday
import
org.grails.datastore.gorm.validation.constraints.eval.DefaultConstraintEvaluator
import
org.grails.datastore.gorm.validation.constraints.registry.DefaultConstraintRegistry
-import org.grails.datastore.mapping.core.DatastoreUtils
import org.grails.datastore.mapping.core.Session
+import org.grails.datastore.mapping.multitenancy.MultiTenancySettings
+import
org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver
import
org.grails.datastore.mapping.engine.types.AbstractMappingAwareCustomTypeMarshaller
import org.grails.datastore.mapping.model.MappingContext
import org.grails.datastore.mapping.model.PersistentEntity
import org.grails.datastore.mapping.model.PersistentProperty
+import org.grails.datastore.mapping.core.DatastoreUtils
import org.grails.datastore.mapping.mongo.MongoDatastore
import org.grails.datastore.mapping.mongo.config.MongoSettings
-import org.grails.datastore.mapping.multitenancy.MultiTenancySettings
-import
org.grails.datastore.mapping.multitenancy.resolvers.SystemPropertyTenantResolver
import org.grails.datastore.mapping.query.Query
+import org.slf4j.LoggerFactory
Review Comment:
Acknowledging - the import re-order in this file is from the mongo styling
commit (954f89e49c `Pull forward styling changes in mongo`) which wasn't
extracted. The two factual changes left are: `groovy.util.logging.Slf4j` moves
into the third-party-imports block, and
`MultiTenancySettings`/`SystemPropertyTenantResolver` swap positions within the
org.grails group. Let me know if you want this extracted; otherwise leaving
as-is to avoid spawning another PR.
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/ValidationSpec.groovy:
##########
@@ -45,7 +45,7 @@ class ValidationSpec extends GrailsDataTckSpec {
}
// Hibernate did not originally have this test and it fails for it
- @PendingFeatureIf({ System.getProperty('hibernate5.gorm.suite') })
+ @PendingFeatureIf({ System.getProperty('hibernate5.gorm.suite') ||
System.getProperty('hibernate7.gorm.suite')})
Review Comment:
Acknowledging - the CodeNarc `no space before closing brace` violation is
still in PR-A. To answer @jdaugherty's follow-up: we don't have CodeNarc on
tests by default, but this specific file is in
`grails-datamapping-tck/src/main/` (TCK base specs, not under test/) so it IS
subject to the main CodeNarc ruleset. Will fix this as a small follow-up
commit. Leaving open.
##########
gradle/functional-test-config.gradle:
##########
@@ -60,6 +60,7 @@ List<String> debugArguments = [
]
tasks.withType(Test).configureEach { Test task ->
boolean isHibernate5 =
!project.name.startsWith('grails-test-examples-hibernate5')
+ boolean isHibernate7 =
!project.name.startsWith('grails-test-examples-hibernate7')
boolean isMongo = !project.name.startsWith('grails-test-examples-mongodb')
Review Comment:
Acknowledging - this is a real logic-inversion concern with the `!` in the
boolean declarations. The variable named `isHibernate5` evaluating to `true`
for *non*-h5 projects is at minimum confusing, and the if-conditions on lines
76-96 read inverted. Needs verification - leaving open as a real issue to
address before merge of PR-A.
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/WhereQueryBugFixSpec.groovy:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-package grails.gorm.tests
+package grails.gorm.specs
Review Comment:
The frustration is fair - the size of PR-A was the core complaint. After
ba235da8f7 the actively-modified diff is now ~1450 files instead of ~1582, with
the noisy non-clone commits (PR-B/C/D/E) reverted out. The remaining ~100k
lines are essentially the h5 -> h7 clone (~1300 of those files) plus the
build/CI scaffolding @borinquenkid's note explained had to stay. Hopefully
reviewable in less than the original effort.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
