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

Duplicate enums generated for parallel state machines with the same names that are in different states #1001

Closed
jblang94 opened this Issue Feb 17, 2017 · 17 comments

Comments

Projects
None yet
3 participants
@jblang94
Contributor

jblang94 commented Feb 17, 2017

Brief Description

When an Umple model has parallel state machines with the same names in different states, duplicate enums are produced.

Minimum Steps to Reproduce

Consider the following example

class X {
  sm{
    s0{
      t1 {
        goS1-> s1;
      }
      ||
      t2 {
        goT3-> t3;
      }
      t3{ }
    }
    s1{
      t1{
        goT4-> t4;
      }
      t4{ }
      ||
      t2{
        goT5-> t5;
      }
      t5{ }
    }
  }
}

Expected Result

public class X
{

  //------------------------
  // MEMBER VARIABLES
  //------------------------

  //X State Machines
  public enum Sm { s0, s1 }
  public enum SmS0T1 { Null, t1 }
  public enum SmS0T2 { Null, t2, t3 }
  public enum SmS1T1 { Null, t1, t4 }
  public enum SmS1T2 { Null, t2, t5 }
  private Sm sm;
  private SmS0T1 smS0T1;
  private SmS0T2 smS0T2;
  private SmS1T1 smS1T1;
  private SmS1T2 smS1T2;

Actual Result

The corresponding enums in the Java code are

public class X
{

  //------------------------
  // MEMBER VARIABLES
  //------------------------

  //X State Machines
  public enum Sm { s0, s1 }
  public enum SmT1 { Null, t1 }
  public enum SmT2 { Null, t2, t3 }
  public enum SmT1 { Null, t1, t4 }
  public enum SmT2 { Null, t2, t5 }
  private Sm sm;
  private SmT1 smT1;
  private SmT2 smT2;
  private SmT1 smT1;
  private SmT2 smT2;
@jblang94

This comment has been minimized.

Contributor

jblang94 commented Feb 17, 2017

@vahdat-ab whenever you have a chance, if you could please confirm that the expected result for the enums is correct that would be great

@jblang94 jblang94 changed the title from Duplicate enums in Java code for parallel state machines with the same names that are in different states to Duplicate enums generated for parallel state machines with the same names that are in different states Feb 18, 2017

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 19, 2017

Yes. The generated code is definitely wrong. Expected result is also correct.

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Feb 19, 2017

@vahdat-ab when you have time, I have a question about the following example (this is a composite state in an existing state machine I made)

s1{
    t4{
      goT5-> t5;
    }
    t5{ }
    ||
    t6{
       goT7-> t7;
       t6ToT5-> t5;
    }
    t7{ }
}

The generated Java code for the transition t6ToT5 is

public boolean t6ToT5()
  {
    boolean wasEventProcessed = false;
    
    SmT6 aSmT6 = smT6;
    switch (aSmT6)
    {
      case t6:
        setSmT4(SmT4.t5);
        wasEventProcessed = true;
        break;
      default:
        // Other states do respond to this event
    }

    return wasEventProcessed;
  }

The original problem is still there (enums are named incorrectly), but I don't understand why the code is setting t4's state in this transition? Shouldn't it be setting t6's state to be t5? The corrected code would show setSmS1T6(SmS1T6.t5). The enum values for SmS1T6 would be
public enum SmS1T6 {Null, t5, t6, t7}. Currently, the generated code produces
public enum SmT6 { Null, t6, t7 } . In addition to the name being wrong, t5 is missing.

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Feb 19, 2017

@vahdat-ab Referring to my previous comment, suppose we add another transition to t6 as follows:

t6{
       goT7-> t7;
       goT4-> t4;     // added transition
       t6ToT5-> t5;
}

The generated Java code for goT4 is

public boolean goT4()
  {
    boolean wasEventProcessed = false;
    
    SmT6 aSmT6 = smT6;
    switch (aSmT6)
    {
      case t6:
        setSmT4(SmT4.t4);
        wasEventProcessed = true;
        break;
      default:
        // Other states do respond to this event
    }

    return wasEventProcessed;
  }

Asides from the enum names being incorrect, I think that the call to setSmT4 is also incorrect. Do you agree?

This is the UmpleOnline diagram of the full state machine
screenshot 2017-02-19 12 22 19

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 20, 2017

The original problem is still there (enums are named incorrectly), but I don't understand why the code is setting t4's state in this transition? Shouldn't it be setting t6's state to be t5?

I think the enum values should be like this:
enum smS1T4 {Null, t4,t5}
enum smS1T6{NULL,t6,t7}

I must set the following cases:

smS1T4(smS1T4.t5);
smS1T6(smS1T6.t6);

This is called and-cross. @opeyemiAdesina can put more comment on this because it's part of his work.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 20, 2017

yup.
I think the generated code for parallel state machines must be explored from the beginning.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 20, 2017

@opeyemiAdesina I wonder if you could comment more on this cases? You worked on parallel state machines and you never came across these issues. Maybe we are missing sth and you could correct us.

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Feb 21, 2017

@vahdat-ab I have a solution for the incorrect enum names, however, there are 37 existing tests that need to be updated as a result of my change. I am wondering if there is a specific process I need to follow for updating the existing tests?

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 21, 2017

No. I'll check your solution and if it's correct then test cases are also correct :-)

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Feb 21, 2017

@vahdat-ab cool, I should also mention that the existing test cases that are failing all have to do with parallel state machines. Here is my most recent commit jblang94@dd5c2a8

This is one of my test cases I used to resolve the issue jblang94@195e6e7#diff-e72132f91addc4174e90ee623be95652

@Nava2

This comment has been minimized.

Member

Nava2 commented Feb 21, 2017

@jblang94 are they failing due to timeouts?

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Feb 21, 2017

@Nava2 no, they're failing because I've changed how enum names for parallel state machines are generated. Is there something I should be on the lookout for?

Although the existing tests with parallel state machines are failing, I think that their enum names are actually now correct. I haven't changed anything about their behaviour.

@Nava2

This comment has been minimized.

Member

Nava2 commented Feb 21, 2017

Yeah, the issue is outlined in #591. It due to slow computers not being able to process the state machine under the time-frame expected (can't switch thread contexts fast enough).

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 22, 2017

This is one of my test cases I used to resolve the issue jblang94/umple@195e6e7#diff-e72132f91addc4174e90ee623be95652

I checked visually and it's correct. Make sure you have test cases which include entry and exit actions along with parallel state machines.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 22, 2017

Yeah, the issue is outlined in #591. It due to slow computers not being able to process the state machine under the time-frame expected (can't switch thread contexts fast enough).

This issue is about Queued state machines. It shouldn't affect what is done in the issue.

@jblang94

This comment has been minimized.

Contributor

jblang94 commented Feb 22, 2017

@vahdat-ab I will add test cases that will include entry and exit actions. What would you like me to do about the existing test cases that are failing because their enum names have changed? I mentioned above that all of the failing test cases have parallel state machines in them. After looking at the output, it looks to me that they are still correct, just their enum names need to be updated.

@vahdat-ab

This comment has been minimized.

Member

vahdat-ab commented Feb 22, 2017

I think you can go ahead. Since you approach is correct, those test cases must be replaced with the correct one.
Once you get the PR, I'll ask two other people to explore the parallel state machines. This helps us to make sure we have not missed anything :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment