search

renatoathaydes / Automaton

Simple framework which allows the testing of Swing and JavaFX2 applications.
Apache License 2.0
100 stars 21 forks source link

Extract table cell value from cell renderer rather than model (Fixes #32) #33

Closed meyertee closed 9 years ago

meyertee commented 9 years ago

Suggested fix incl. test for https://github.com/renatoathaydes/Automaton/issues/32

It reads the cell's value from the renderer component, if it is a JLabel (as is the case in the DefaultTableCellRenderer). If the renderer component isn't a JLabel it returns the model value itself as previously.

This may break existing tests if they rely on the difference between model value and rendered value. But I can't think of a reason why you'd want that other than to work around the issue itself.

meyertee commented 9 years ago

Cool, I'll look into those!

renatoathaydes commented 9 years ago

Very nice PR, thanks for the effort! Please have a look at my comments before I can merge this.

I have run the Swing tests and everything seems to be passing! Why do you think this may break some test?

meyertee commented 9 years ago

Thanks! I meant tests that another developer has written for their software using Automaton, i.e. it might be a breaking change. People might be testing against their model values rather than the values that are rendered. Just something to keep in mind.. but maybe differences between value und rendered text aren't very common?

renatoathaydes commented 9 years ago

That's a good point... but Automaton is meant to be used by testers who need to test what they can actually see, not what the underlying Table model or whatever looks like... so this change is a big improvement in any case.

meyertee commented 9 years ago

Done! I just noticed that in listItem2FakeComponent there's also a listComp instanceof JLabel check. Maybe we should also change it to callMethodIfExists(rendererComp, 'getText')? I can chip that in if you want, although it doesn't really belong in this PR, I'll open another :)

renatoathaydes commented 9 years ago

Great! Feel free to change that other call. Will merge from home tonight (I hope).

renatoathaydes commented 9 years ago

You may have noticed that I improved the implementation of callMethodIfExists in master... merge master in your branch just to be certain it works and has no conflicts...

meyertee commented 9 years ago

Merged master, everything is fine! I created a PR for the list change: https://github.com/renatoathaydes/Automaton/pull/34