Feature insert code eating features?

[simoncozens]

I just started trying to fix #506 (at least the part where abvm/blwm/mark are having classes placed in the wrong order), and I noticed something odd in my test failures:

E           @kern2.Default.foo = [period];
E           @kern2.Latn.foo = [b];
E         +
E         + feature ss01 {
E         +     sub a by period;
E         +     # Make period be both Latn and Zyyy.
E         + } ss01;
E
E           lookup kern_Latn {

Wait, what, is the kern feature writer hallucinating entire features? No, it turns out that we add that code to the feature file when testing:

ufo2ft/tests/featureWriters/kernFeatureWriter_test.py

Lines 1380 to 1392 in 87c6881

	# Making a common glyph implicitly have an explicit script assigned (GSUB
	# closure) will still keep it in the common section.
	features = dedent(
	"""
	feature ss01 {
	sub a by period; # Make period be both Latn and Zyyy.
	} ss01;
	"""
	)
	ufo = makeUFO(FontClass, glyphs, groups, kerning, features)
	newFeatures = KernFeatureWriterTest.writeFeatures(ufo)
	assert dedent(str(newFeatures)).lstrip("\n") == expectation

but when we set our test expectations, it's not there:

ufo2ft/tests/featureWriters/kernFeatureWriter_test.py

Lines 1326 to 1335 in 87c6881

	expectation = dedent(
	"""\
	@kern1.Default.foo = [period];
	@kern1.Latn.foo = [a];
	@kern2.Default.foo = [period];
	@kern2.Latn.foo = [b];
	lookup kern_Latn {
	lookupflag IgnoreMarks;
	pos a a 1;

I don't understand this. To me it looks like the kern feature writer is eating the ss01 feature, and our tests ensure that it does get eaten. Am I reading this wrong?

[anthrotype]

Only the new features are returned from writeFeatures method

[simoncozens]

Ah, OK, so the tests have an assumption that new things are added to the end of the file. My thought was to place all the definitions right at the top. Anyway, that makes more sense; thank you!

[anthrotype]

it's defined in

ufo2ft/tests/featureWriters/__init__.py

Lines 9 to 20 in 87c6881

	@classmethod
	def writeFeatures(cls, ufo, **kwargs):
	"""Return a new FeatureFile object containing only the newly
	generated statements, or None if no new feature was generated.
	"""
	writer = cls.FeatureWriter(**kwargs)
	feaFile = parseLayoutFeatures(ufo)
	n = len(feaFile.statements)
	if writer.write(ufo, feaFile):
	new = ast.FeatureFile()
	new.statements = feaFile.statements[n:]
	return new

however, now I'm thinking that assuming that only the lines following len(feaFile) are new might not actually be true all the time esp. with insertion markers and such

[simoncozens]

I'm rewriting it to look for "really new" statements (i.e. not in the file before), regardless of where they were. This will allow me to then put definitions at the top.