Change of behavior regarding defines?

[bjosv]

Hi!
When delivering a no-code PR to another project that uses Elvis I suddenly got a DRY error in the code. The travis build used the latest elvis master version and this indicates there has been a behavioral change in the later commits of elvis.

The issue I got was triggered by test code within src-files conditioned with `-ifdef(TEST).
It seems that elvis now checks all source code indifferent of ifdef's.

I did some digging and created a code example to run elvis on, to reproduce the issue:
https://github.com/bjosv/elvis_issue

• When running elvis 0.4.2 (088b7ac)
  elvis rock
  i.e no issue

• When running elvis from master (c55c59b)
  elvis rock
# src/elvis_issue.erl [FAIL]
- dont_repeat_yourself
- The code in the following (LINE, COL) locations has the same structure: (13, 65), (16, 65).
  This is part of the ifdef'ed test code.

What I have seen there are these interesting changes from 0.4.2 to master:

• elvis uses a later elvis_core
  • elvis_core depends on katana-code
    • katana-code was lifted from 0.1.2 to 0.2.1
      • which replaced aleppo with epp_dodger in [#26] Drop aleppo and use a forked epp_dodger katana-code#31

and this changes how the node-tree representing the code is created. The content seems to differs regarding ifdef's.

According to #321 elvis might ignore conditional compilation construct since a long time, but not in all cases as it seems.

Is elvis expected to handle conditionals, i.e ignore code within TEST scope?

• If it does; there seems to be a regression that can be triggered by the repo above.
• If not; there seems to be a bigger change under the hood that might be good to highlight in the next release note.

Thanks!

[elbrujohalcon]

Hi @bjosv.
Wow! #321 is an old one 😓
In any case, the change happened when @jfacorro updated katana-code to use ktn_dodger instead of aleppo.
The idea of checking as much code as possible (including the code that's behind macro conditionals like -ifdef) was always our intention, but it was not possible with aleppo.
You're right that we didn't highlight that change when we released the version with the updated reference to katana-code. Truth is we didn't realize that was going to happen, since we changed katana-code preprocessor for a different reason.
We'll try to make it more prominent for future releases.

[jfacorro]

@bjosv Thank you for the comprehensive description in the issue 🥰.

As @elbrujohalcon says we missed this change in behaviour and therefore failed to let people know. We should definitely add a test case that reflects the expected behaviour from elvis in these cases.

[paulo-ferraz-oliveira]

@jfacorro, is something missing here, like the test case you talk about? Or can this be closed?

[jfacorro]

@paulo-ferraz-oliveira I think I had the plan to include this new behaviour in the README or somewhere else where it made sense, but couldn't think of an ideal place.