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

Fix TestCompilerTests with locales other than english #31407

Closed
wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Oct 11, 2023

		assertThatExceptionOfType(CompilationException.class).isThrownBy(
				() -> TestCompiler.forSystem().failOnWarning().withSources(
						SourceFile.of(HELLO_DEPRECATED), main).compile(compiled -> {
				})).withMessageContaining("warnings found and -Werror specified");

is failed since error message may not be english before this commit.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 11, 2023
@quaff
Copy link
Contributor Author

quaff commented Oct 11, 2023

It's introduced by 4b14a0b @snicoll

@snicoll snicoll added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 11, 2023
@snicoll snicoll self-assigned this Oct 11, 2023
@snicoll snicoll added this to the 6.1.0-RC1 milestone Oct 11, 2023
@snicoll
Copy link
Member

snicoll commented Oct 11, 2023

Ah good catch, @quaff thank you. I think I am probably going to polish that to use a different assertion.

@snicoll
Copy link
Member

snicoll commented Oct 11, 2023

Turns out I think we should fix this differently. Thanks for the report but we'll follow-up on #31408.

@snicoll snicoll closed this Oct 11, 2023
@snicoll snicoll added the status: superseded An issue that has been superseded by another label Oct 11, 2023
@snicoll snicoll removed this from the 6.1.0-RC1 milestone Oct 11, 2023
@quaff
Copy link
Contributor Author

quaff commented Nov 1, 2023

@snicoll I'm afraid your fix doesn't work, It still report localized error message.

> Task :spring-core-test:test FAILED
TestCompilerTests > compileWhenSourceUseDeprecateCodeAndFailOnWarningIsSet() FAILED
    java.lang.AssertionError at TestCompilerTests.java:176

@snicoll
Copy link
Member

snicoll commented Nov 1, 2023

Damn, thanks for testing and reporting back. I wonder how it is different from your fix. The only thing I've done is not hardcoding the locale but rather make it configurable. What did I miss?

quaff added a commit to quaff/spring-framework that referenced this pull request Nov 1, 2023
Fix TestCompilerTests with locales other than english

See spring-projectsGH-31407
@quaff
Copy link
Contributor Author

quaff commented Nov 1, 2023

1
2

diagnostic is already localized.
I created #31532 to fix it.

@snicoll
Copy link
Member

snicoll commented Nov 1, 2023

diagnostic is already localized.

That's odd. Then Diagnostic#getMessage(Locale) is not reliable. Rather than introducing another way to do the assertion, I think we need to fix how the locale is specified. Adding the code makes the Locale argument useless.

I think the PR should rather change this.compiler.getStandardFileManager to specify the Locale in the second argument. Can you try that since you have a way to easily reproduce?

@quaff
Copy link
Contributor Author

quaff commented Nov 1, 2023

diagnostic is already localized.

That's odd. Then Diagnostic#getMessage(Locale) is not reliable. Rather than introducing another way to do the assertion, I think we need to fix how the locale is specified. Adding the code makes the Locale argument useless.

I think the PR should rather change this.compiler.getStandardFileManager to specify the Locale in the second argument. Can you try that since you have a way to easily reproduce?

		StandardJavaFileManager standardFileManager = this.compiler.getStandardFileManager(
				null, this.locale, null);

it doesn't work.

@snicoll
Copy link
Member

snicoll commented Nov 1, 2023

CompilationTask#setLocale then?

@quaff
Copy link
Contributor Author

quaff commented Nov 1, 2023

CompilationTask#setLocale then?

	private DynamicClassLoader compile() {
		ClassLoader classLoaderToUse = (this.classLoader != null ? this.classLoader
				: Thread.currentThread().getContextClassLoader());
		List<DynamicJavaFileObject> compilationUnits = this.sourceFiles.stream().map(
				DynamicJavaFileObject::new).toList();
		StandardJavaFileManager standardFileManager = this.compiler.getStandardFileManager(
				null, this.locale, null);
		DynamicJavaFileManager fileManager = new DynamicJavaFileManager(
				standardFileManager, classLoaderToUse, this.classFiles, this.resourceFiles);
		if (!this.sourceFiles.isEmpty()) {
			Errors errors = new Errors(this.locale);
			CompilationTask task = this.compiler.getTask(null, fileManager, errors,
					this.compilerOptions, null, compilationUnits);
			task.setLocale(this.locale);
			if (!this.processors.isEmpty()) {
				task.setProcessors(this.processors);
			}
			boolean result = task.call();
			if (!result || errors.hasReportedErrors()) {
				throw new CompilationException(errors.toString(), this.sourceFiles, this.resourceFiles);
			}
		}
		return new DynamicClassLoader(classLoaderToUse, this.classFiles, this.resourceFiles,
				fileManager.getDynamicClassFiles(), fileManager.getDynamicResourceFiles());
	}

Not working.

@snicoll
Copy link
Member

snicoll commented Nov 1, 2023

I went down a rabbit hole and I am not sure why this does not work. It goes all the way down to ResourceBundle.getBundle("com.sun.tools.javac.resources.compiler", Locale.ENGLISH) that returns the one of the default locale no matter what. Perhaps it's a module visibility problem?

In any case. I've reverted the support in #31536 and relaxed the assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants