Copilot commented on code in PR #16643: URL: https://github.com/apache/grails-core/pull/16643#discussion_r4264458765
########## grails-shell-cli/src/test/groovy/org/grails/cli/command/CommandRunnerSpec.groovy: ########## @@ -0,0 +1,267 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.cli.command + +import org.springframework.boot.cli.command.Command as BootCommand +import org.springframework.boot.cli.command.CommandException as BootCommandException +import org.springframework.boot.cli.command.HelpExample as BootHelpExample +import org.springframework.boot.cli.command.options.OptionHelp +import org.springframework.boot.cli.command.status.ExitStatus +import spock.lang.Specification +import spock.util.environment.RestoreSystemProperties + +@RestoreSystemProperties +class CommandRunnerSpec extends Specification { + + ByteArrayOutputStream output = new ByteArrayOutputStream() + ByteArrayOutputStream errors = new ByteArrayOutputStream() + PrintStream originalOut + PrintStream originalErr + + void setup() { + originalOut = System.out + originalErr = System.err + System.setOut(new PrintStream(output, true, 'UTF-8')) + System.setErr(new PrintStream(errors, true, 'UTF-8')) + } + + void cleanup() { + System.out.close() + System.err.close() + System.setOut(originalOut) + System.setErr(originalErr) Review Comment: Closing `System.out`/`System.err` (even when temporarily replaced) can interfere with other tests/tools writing to stdout/stderr in the same JVM, especially if the suite is ever run concurrently. Prefer restoring the original streams first and then closing only the capturing `PrintStream` instances you created (or omit closing entirely since they wrap in-memory buffers). ########## grails-shell-cli/src/test/groovy/org/grails/cli/profile/commands/io/FileSystemInteractionImplSpec.groovy: ########## @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.cli.profile.commands.io + +import grails.build.logging.GrailsConsole +import grails.util.BuildSettings +import org.grails.cli.profile.ExecutionContext +import org.grails.io.support.FileSystemResource +import spock.lang.Specification +import spock.lang.TempDir + +class FileSystemInteractionImplSpec extends Specification { + + @TempDir + File directory + + FileSystemInteractionImpl interaction + PrintStream originalOut + PrintStream capturedOut + + void setup() { + def context = Stub(ExecutionContext) { getBaseDir() >> directory } + interaction = new FileSystemInteractionImpl(context) + originalOut = GrailsConsole.instance.out + capturedOut = new PrintStream(new ByteArrayOutputStream()) + GrailsConsole.instance.out = capturedOut + } + + void cleanup() { + GrailsConsole.instance.out = originalOut + capturedOut.close() + } Review Comment: This spec mutates `GrailsConsole.instance.out` (a shared singleton) for the duration of each test. If the test suite is configured to run specs in parallel, this creates a race with other tests that also use `GrailsConsole.instance`. Consider isolating the spec (e.g., Spock `@Isolated`) or refactoring to avoid global singleton mutation (inject a console/output where possible). ########## grails-shell-cli/src/test/groovy/org/grails/cli/profile/commands/templates/TemplateRendererImplSpec.groovy: ########## @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.cli.profile.commands.templates + +import grails.build.logging.GrailsConsole +import org.grails.cli.profile.ExecutionContext +import org.grails.cli.profile.ProfileRepository +import org.grails.io.support.FileSystemResource +import spock.lang.Specification +import spock.lang.TempDir + +class TemplateRendererImplSpec extends Specification { + + @TempDir + File directory + + GrailsConsole console = Mock(GrailsConsole) + ProfileRepository repository = Mock(ProfileRepository) + TemplateRendererImpl renderer + + void setup() { + def context = Stub(ExecutionContext) { + getBaseDir() >> directory + getConsole() >> console + } + renderer = new TemplateRendererImpl(context, repository) + } + + def 'text templates render their model as UTF-8 and create parent directories'() { + given: + def destination = new File(directory, 'nested/greeting.txt') + + when: + renderer.render('Hello ${name}!', destination, [name: 'Grails café']) + + then: + destination.getText('UTF-8') == 'Hello Grails café!' + 0 * repository._ + } + + def 'existing destinations are preserved unless overwrite is requested'() { + given: + def destination = new File(directory, 'greeting.txt') + destination.text = 'original' + + when: + renderer.render('Hello ${name}!', destination, [name: 'Grails']) + + then: + destination.text == 'original' + 1 * console.warn('Destination file greeting.txt already exists, skipping...') + + when: + renderer.render('Hello ${name}!', destination, [name: 'Grails'], true) + + then: + destination.text == 'Hello Grails!' + 0 * repository._ Review Comment: This test uses Groovy's `File.text` (platform default charset) for both setup and assertions, while other coverage in this spec explicitly verifies UTF-8 behavior. On non-UTF-8 default platforms this can make the test flaky/inconsistent with the intended contract. Prefer `setText(..., 'UTF-8')` / `getText('UTF-8')` (or consistently use `getText('UTF-8')` for assertions) in this feature. ########## grails-shell-cli/src/test/groovy/org/grails/cli/profile/commands/templates/TemplateRendererImplSpec.groovy: ########## @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.cli.profile.commands.templates + +import grails.build.logging.GrailsConsole +import org.grails.cli.profile.ExecutionContext +import org.grails.cli.profile.ProfileRepository +import org.grails.io.support.FileSystemResource +import spock.lang.Specification +import spock.lang.TempDir + +class TemplateRendererImplSpec extends Specification { + + @TempDir + File directory + + GrailsConsole console = Mock(GrailsConsole) + ProfileRepository repository = Mock(ProfileRepository) + TemplateRendererImpl renderer + + void setup() { + def context = Stub(ExecutionContext) { + getBaseDir() >> directory + getConsole() >> console + } + renderer = new TemplateRendererImpl(context, repository) + } + + def 'text templates render their model as UTF-8 and create parent directories'() { + given: + def destination = new File(directory, 'nested/greeting.txt') + + when: + renderer.render('Hello ${name}!', destination, [name: 'Grails café']) + + then: + destination.getText('UTF-8') == 'Hello Grails café!' + 0 * repository._ + } + + def 'existing destinations are preserved unless overwrite is requested'() { + given: + def destination = new File(directory, 'greeting.txt') + destination.text = 'original' + + when: + renderer.render('Hello ${name}!', destination, [name: 'Grails']) + + then: + destination.text == 'original' + 1 * console.warn('Destination file greeting.txt already exists, skipping...') + + when: + renderer.render('Hello ${name}!', destination, [name: 'Grails'], true) + + then: + destination.text == 'Hello Grails!' + 0 * repository._ + } + + def 'file and resource templates render through their public overloads'() { + given: + def template = new File(directory, 'greeting.tpl') + template.setText('Hello ${name}!', 'UTF-8') + def fileDestination = new File(directory, 'file.txt') + def resourceDestination = new File(directory, 'resource.txt') + + when: + renderer.render(template, fileDestination, [name: 'File']) + renderer.render(new FileSystemResource(template), resourceDestination, [name: 'Resource']) + + then: + fileDestination.text == 'Hello File!' + resourceDestination.text == 'Hello Resource!' Review Comment: Assertions again use `File.text` (default charset). Since the template is explicitly written as UTF-8 and the renderer is expected to output UTF-8, these assertions should read with `getText('UTF-8')` to avoid platform-dependent failures. -- 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]
