Bug 263081

Comment 1 Aghiles Abdesselam 2009-01-30 13:42:55 EST

Created attachment 124314 [details]
new junits

Comment 2 Kevin McGuire 2009-01-31 18:17:18 EST

Ralf, Aghiles wrote some tests for cascading order.  I've not reviewed them (feel free to if you get to them before I do).

Comment 3 Kevin McGuire 2009-01-31 18:46:27 EST

Aghiles, Ralf wrote some cascading tests too, see CascadeTest.java in org.eclipse.e4.ui.tests.css.core.parser.

- Please review for overlap and see what additional tests you're providing. 

- You'll notice Ralf's test only depends on Core, not SWT.  Yours should be core tests too since they test the parser, not the SWT bindings.  Thus for tests which you're providing above his, please rework them to not need SWT.  I suggest you follow his example.

- Assuming you have additional tests above what Ralf did, please resubmit the patch with the changes described, marking the current one obsolete.

Comment 4 Aghiles Abdesselam 2009-02-03 13:06:52 EST

Created attachment 124581 [details]
additional tests for the css junits

additional tests for the css junits

Comment 5 Aghiles Abdesselam 2009-02-12 14:07:05 EST

Created attachment 125559 [details]
updated CSS junits

Comment 6 Kevin McGuire 2009-02-13 17:29:50 EST

Is patch #2 obsolete? (If yes then please mark as)

Comment 7 Kevin McGuire 2009-02-13 17:39:18 EST

I noticed that many tests, eg:

testTestFontItalic
testTestFontBold
testBackgroundNameColor

Are almost identical except for the style sheet and the name of the property being retrieved.

Can we create a helper method for all the common code, just passing in the style sheet and property name?  So the resulting test would look something like:

	public void testBackgroundNameColor() throws Exception{
		CSSValue value = testProperty(
			"Label { background-color: green }", 
			"background-color");
		assertTrue(value instanceof CSSPrimitiveValue);
		String colorString = ((CSSPrimitiveValue) value).getStringValue();
		assertEquals("green", colorString);
	}

Comment 8 Kevin McGuire 2009-02-13 17:46:36 EST

Checking equality of floats is a bit odd because of their imprecision.  I can see why you ended up with like this though:

		assertEquals(255.0f, colorValue.getRed().getFloatValue(
				CSSPrimitiveValue.CSS_NUMBER), 0f);

because there's no getIntValue().

However, in looking at callers of getFloatValue(), it looks like we always just cast to an (int).  Perhaps that's what the test should do, since at least then:

1) The test would match how we make use of the return value in practice
2) We'd avoid the testing of equality of floats.

Comment 9 Kevin McGuire 2009-02-13 18:00:42 EST

Regarding CSSSelectorPrecedence:

1) I'm not clear on the relationship between it and CascadeTest.  Both have a testBug261081 and are testing the rule precedence.

2) We don't need to write an SWT based test for this kind of test (hence, CascadeTest in css.core). 

------------------------
Regarding CTabFolderAddedProperties:

1) It should just be called CTabFolderTest.

2) It shouldn't be part of this patch, since the code to implement these properties is in a different bug.

3) It has commented out code.  Please remove this from the patch.

4) It should be combined with the existing CTabFolderTest (even if you wanted to keep something like CTabFolderAddedProperties, you'd need to move a few of the tests from CTabFolderTest in there like testMaximizeVisible()).


------------------------
Regarding WidgetBehaviour:

1) Odd name, seems to be testing having multiple selectors within a single rule, 
   Label, Button, Shell  { ... }

Shouldn't it then be a core test, since we don't need SWT widgets for it?

Comment 10 Kevin McGuire 2009-02-13 18:01:06 EST

Please address the comments above and resubmit patch, thanks.

Comment 11 Kevin McGuire 2009-02-13 18:05:08 EST

Btw, CascadingTest.testImportantRule is failing.  I assume this is an expected failure?  ie. the test correctly specifies expected behaviour but the css code doesn't implement it correctly?

Comment 12 Kevin McGuire 2009-02-13 18:08:06 EST

Aghiles, many of the SWT tests you added are failing.  
Did you run these tests?  The tests look wrong.  For example:

		CTabFolder folderToTest = createTestCTabFolder("Label { font: Arial 12px; font-weight: bold }");
		assertEquals(1, folderToTest.getFont().getFontData().length);
		FontData fontData = folderToTest.getFont().getFontData()[0];
		assertEquals("Arial", fontData.getName());

I'm assuming this was a copy/paste error, and the first line should've read:

		CTabFolder folderToTest = createTestCTabFolder("CTabFolder { font: Arial 12px; font-weight: bold }");

Comment 13 Aghiles Abdesselam 2009-02-17 15:47:30 EST

Comment on attachment 125559 [details]
updated CSS junits

Patch will be included in bug 265214

Comment 14 Kevin McGuire 2009-02-17 18:44:11 EST

Closed as part of bug #265214