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

Support for broadcoms multifunctional devices #231

Closed
wants to merge 22 commits into from

Conversation

Projects
None yet
3 participants
@mvidner
Copy link
Member

commented Aug 14, 2014

Supersedes #227.

I have shown what I think is a better test,
and merged with master to be mergeable,

mchf and others added some commits Aug 11, 2014

@mchf

This comment has been minimized.

Copy link
Member

commented on 5f4b758 Aug 14, 2014

I wanted to create generic probe helper. Your approach means that when I want to use the probe helper elsewhere I'll probably need to touch the helper even fix this test. Also I currently don't see any reason (of course there can be) for stubbing Arch ... In general I prefer minimal stubbing.

@mvidner

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2014

I'll probably need to touch the helper even fix this test.

Definitely not. Note that the factory code is already shared by 2 tests, and instead of changing the factory to support storageonly I've merged the attribute to its result.

As for the original 500-line data

  1. YAGNI - only add code when you need it.
  2. It is spaghetti data. Noone knows what is important and what is not.

The minimal factory approach makes it explicit what each test case needs.

@mvidner

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2014

"Minimal stubbing" can be minimal in different ways: minimum code in test to stub it (your version), or minimum number of calls stubbed. I think the latter also has value, especially if it is easy to achieve. But that is a minor point for me.

@mchf

This comment has been minimized.

Copy link
Member

commented Aug 14, 2014

When I need to use the factory in another test I'll most probably need to use different attributes. So, I'll need to touch this code or use another factory. I did it "my way" bcs I can add device description e.g. obtained from customer easily and all tests would easily use it with no touching. The helper was not about an algorithm but a sort of database which should be filled by data to cover various edge cases which might come.

So I definitely have a different approach than you. I wanted to create large read-only data base e.g. filled by real cases to be used as data input, you want it minimal and add special attributes as needed.

I've no more comments to the code.

# card_ok = Arch::is_xenU () || Arch::ppc();
else
Ops.set(one, "drivers", drivers)
if drivers == []

This comment has been minimized.

Copy link
@lslezak

lslezak Aug 14, 2014

Member

What about drivers.empty? ?

@lslezak

This comment has been minimized.

Copy link
Member

commented Aug 14, 2014

So, I'll need to touch this code or use another factory.

Just make the factory parametrized, like here: https://github.com/yast/yast-registration/blob/master/test/factories.rb#L4

There I can ask the factory for a generic product, or I can override specific attributes to create product I need in specific test.

@mchf

This comment has been minimized.

Copy link
Member

commented on test/factories/probe_netcard.rb in 5f4b758 Aug 14, 2014

num should be converted into hex here and checked if it is num <= 0xF

@mchf

This comment has been minimized.

Copy link
Member

commented Aug 14, 2014

Note:
Disagreement is about test data. There are two possibilities

  1. I'd prefer to use real word data e.g. copied from logs and so on. See PR#227
  2. @mvidner created the factory which is presented here.
@mchf

This comment has been minimized.

Copy link
Member

commented Aug 14, 2014

Just make the factory parametrized, like here: https://github.com/yast/yast-registration/blob/master/test/factories.rb#L4

then I'd need to use more than 20 parameters. What I mean is that there are plenty of devices, many vendors and lot of state information present in one device description. And there are already an edge cases in the code depending on arch, device type and so on. If the factory only changes device name, mac address and device description it simply is not enough in my POV

@mvidner

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2014

We'll see which approach is more suitable as we add more tests and fixtures. This may be resurrected or not.

@mvidner mvidner closed this Aug 29, 2014

@mvidner mvidner deleted the bnc841170-broadcom branch Sep 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.