haxe-checkstyle failures

[Simn]

    FAIL: massive.munit.AssertionException: Value [null] was not equal to expected value [eol] at checkstyle.detect.DetectCodingStyleTest#testDetectOperatorWrap (486)
    FAIL: massive.munit.AssertionException: Value [null] was not equal to expected value [^[A-Z][A-Z0-9]*(_[A-Z0-9_]+)*$] at checkstyle.detect.DetectCodingStyleTest#testDetectConstantName (322)
    FAIL: massive.munit.AssertionException: Value [null] was not equal to expected value [eol] at checkstyle.detect.DetectCodingStyleTest#testDetectSeparatorWrap (506)

@AlexHaxe's hint:

might it be an issue, that all three of these checks are child classes and the failing property comes from base class?
at least that's what all three have in common and (I think) all others don't

[AlexHaxe]

second hint (because check detection logic might be a bit complex): check detection calls check.configureProperty which uses Reflect.setField on checks under detection to run them with different settings.
The same configureProperty is used after reading a checkstyle configuration file during a "normal" checkstyle run.

[Simn]

I just checked and this is still a problem.

[Simn]

I've changed one of the tests like so:

	@Test
	public function testDetectSeparatorWrap() {
		var check = new SeparatorWrapCheck();
		var detectedChecks:Array<CheckConfig> = DetectCodingStyle.detectCodingStyle([check], [buildCheckFile(SAMPLE_CODING_STYLE)]);
		Assert.areEqual(1, detectedChecks.length);
		Assert.areEqual("SeparatorWrap", detectedChecks[0].type);
		var props = cast detectedChecks[0].props;
		Sys.println(props); // {}
		Sys.println(check.option); // eol
		Sys.println(props.option); // null
		Assert.areEqual(WrapCheckBaseOption.EOL, props.option);
	}

So the expected value is on the original instance of SeperatorWrapCheck, but the props object is empty.

I've noticed that ConfigUtils.makeCheckConfig uses Reflect.fields(check), which is not guaranteed to work for classes:

  This method is only guaranteed to work on anonymous structures. Refer to
  `Type.getInstanceFields` for a function supporting class instances.

[Simn]

Yes the tests pass after changing that to for (prop in Type.getInstanceFields(Type.getClass(check))) {. But then the non-genjvm tests fail with Cannot access field for writing.

Anyway, genjvm is within the spec here, but to be honest I'm not sure why we can't just make Reflect.fields call Type.getInstanceFields if the passed object is a Haxe-class.

[AlexHaxe]

I guess I should have read the docs more carefully instead of going with what worked when writing that function.

getInstanceFields works but an extra Reflect.isFunction is needed to skip putting functions into the resulting check configuration.

[AlexHaxe]

fixed in HaxeCheckstyle/haxe-checkstyle#498

note: I rearranged build files a bit, there is now a testJava.hxml and a testJvm.hxml

[Simn]

Alright, thanks for the info! I also realized that I would have to add this isFunction check if I wanted to call getInstanceFields from Reflect.fields. That seems pretty heavy, so I prefer to not do it and point to the documentation.

I would only consider this if all targets already worked like that, but I don't think that's the case. You probably just got lucky that it works on the targets you were using.